Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Cleanup LowerBlockStore#27170

Merged
CarolEidt merged 3 commits into
dotnet:masterfrom
mikedn:lower-cleanup
Oct 15, 2019
Merged

Cleanup LowerBlockStore#27170
CarolEidt merged 3 commits into
dotnet:masterfrom
mikedn:lower-cleanup

Conversation

@mikedn
Copy link
Copy Markdown

@mikedn mikedn commented Oct 13, 2019

Extracted from #27035

Fixes #27169

@mikedn mikedn changed the title Cleanup LowerBlockStore [WIP] Cleanup LowerBlockStore Oct 14, 2019
@mikedn
Copy link
Copy Markdown
Author

mikedn commented Oct 14, 2019

@sandreenko Is it possible to re-run only "Run Test Pri0 Linux arm64 checked"? I would like to check that the test I added catches the bug before adding the actual fix.

@sandreenko
Copy link
Copy Markdown

sandreenko commented Oct 14, 2019

@sandreenko Is it possible to re-run only "Run Test Pri0 Linux arm64 checked"? I would like to check that the test I added catches the bug before adding the actual fix.

I think we don't have that precision(and probably don't need), so I have restarted all failed checks.

Should not Run Test Pri0 Windows_NT arm64 checked repro that as well?

@mikedn
Copy link
Copy Markdown
Author

mikedn commented Oct 14, 2019

Should not Run Test Pri0 Windows_NT arm64 checked repro that as well?

Sure, if that does anything. I looked at a successful build and noticed that the "Send tests to Helix" step is actually skipped in that job so I'm not sure what that job does, if anything.

Thanks!

@mikedn
Copy link
Copy Markdown
Author

mikedn commented Oct 14, 2019

Cool, test failed: https://dev.azure.com/dnceng/public/_build/results?buildId=386645&view=ms.vss-test-web.build-test-results-tab&runId=11977646&resultId=100695&paneView=debug

@mikedn mikedn changed the title [WIP] Cleanup LowerBlockStore Cleanup LowerBlockStore Oct 14, 2019
@sandreenko sandreenko requested a review from a team October 14, 2019 23:04
sandreenko
sandreenko approved these changes Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CarolEidt
CarolEidt approved these changes Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

Comment thread src/jit/lowerarmarch.cpp
assert(dstAddr->gtType == TYP_BYREF || dstAddr->gtType == TYP_I_IMPL);
// TODO-Cleanup: Make sure that GT_IND lowering didn't mark the source address as contained.
// Sometimes the GT_IND type is a non-struct type and then GT_IND lowering may contain the
// address, not knowing that GT_IND is part of a block op that has containment restrictions.
Copy link
Copy Markdown

@CarolEidt CarolEidt Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this clarifying comment!

@CarolEidt CarolEidt merged commit 5b95b4a into dotnet:master Oct 15, 2019
@mikedn mikedn deleted the lower-cleanup branch October 21, 2019 07:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@CarolEidt CarolEidt CarolEidt approved these changes

@sandreenko sandreenko sandreenko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

initblk incorrectly handles 0 fill on ARM64

3 participants

@mikedn @sandreenko @CarolEidt