-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bad folding #54722
Fix bad folding #54722
Conversation
GT_CNS_INT of TYP_INT on 64 hosts has an implicit contract: the value returned by IconValue() must "fit" into 32 bit signed integer, i. e. "static_cast<int>(IconValue()) == IconValue()". gtFoldExprConst was failing to uphold this contract when the target was 32 bit and the host was 64 bit. Fix this by always truncating before calling SetIconValue().
55a8412
to
576315f
Compare
Sorry for a late response, the change looks good, but I would like to repro the issue.
It should be pretty straightforward with this instruction: https://github.com/dotnet/runtime/blob/main/docs/workflow/debugging/coreclr/debugging-crossgen2.md you run your test with |
@sandreenko Hmm, I suppose I was not really clear with the wording. What I meant is how to make sure the test is run the right way in CI, locally it can be reproduced easily with the usual AltJit setup. That said, I have now checked that it does indeed reproduce with CG2 too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Will rerun the CI one more time... |
Well, that's not so many failures, so, progress :). 1)
2)
3)
|
I saw these failures in other PRs, the infra is in a bad state, azure is trying to patch it up, but for now we have to merge on red. |
GT_CNS_INT
ofTYP_INT
on 64 hosts has an implicit contract: the value returned byIconValue()
must "fit" into 32 bitsigned integer, i. e.
static_cast<int>(IconValue()) == IconValue()
.gtFoldExprConst
was failing to uphold this contract when the target was 32 bit and the host was 64 bit.Fix this by always truncating before calling
SetIconValue()
.I also added a test that reproduces the bad behavior, but to be effective it needs to be CG'ed on a 64 bit machine for a 32 bit target. I am not sure how to make sure that happens...
This change is not zero-diff. There were ~10 cases of either bad codegen or missed folding in the SPMI collections (all available for win-64 -> win-x86 cross-compilation), though none in the product code.
Found while looking at #53983.