Fix duplicated FldSeq in block morphing #62687
Conversation
Reusing the address for the first field is problematic as the first field is likely to be at a zero offset, thus a zero-offset field sequence will be attached to it, and subsequent copies of the same tree will pick it up, which is incorrect. Fix by reusing the address for the last field instead.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsReusing the address for the first field is problematic as the first field is likely to be at a zero offset, thus a zero-offset field sequence will be attached to it, and subsequent copies of the same tree will pick it up, which is incorrect. Fix by reusing the address for the last field instead. [000015] -A-X-+------ * COMMA void
-[000008] -A-X-------- +--* ASG long
-[000006] *--X---N---- | +--* IND long
-[000000] -----+------ | | \--* LCL_VAR byref V00 arg0 Zero Fseq[FirstLngValue]
-[000007] ------------ | \--* LCL_VAR long V03 tmp1
+[000009] -A-X-------- +--* ASG long
+[000007] *--X---N---- | +--* IND long
+[000006] -----+------ | | \--* LCL_VAR byref V00 arg0 Zero Fseq[FirstLngValue]
+[000008] ------------ | \--* LCL_VAR long V03 tmp1
[000014] -A-X-------- \--* ASG long
[000012] *--X---N---- +--* IND long
[000011] ------------ | \--* ADD byref
-[000009] -----+------ | +--* LCL_VAR byref V00 arg0 Zero Fseq[FirstLngValue]
+[000000] -----+------ | +--* LCL_VAR byref V00 arg0
[000010] ------------ | \--* CNS_INT long 8 Fseq[SecondLngValue]
[000013] ------------ \--* LCL_VAR long V04 tmp2Part of #58312. No diffs are expected.
|
SingleAccretion
force-pushed
the
Fix-Duplicated-FldSeq-In-MorphBlock
branch
from
cdf0f98
to
f48008b
Compare
|
Will request the @dotnet/jit-contrib |
Rerunning. |
| { | ||
| // Use the orginal m_dstAddr tree when i == 0 | ||
| // Reuse the orginal m_dstAddr tree for the last field. |
There was a problem hiding this comment.
Should we assert that we're not reusing an address with a zero offset field seq?
There was a problem hiding this comment.
Hmm, it is legal for the address to contain a zero-offset sequence before the morphing, so I suppose asserting here would require:
- Getting the "base" sequence (essentially perform the reverse of what
fgAddFieldSeqForZeroOffsetdoes). - After morphing, loop over all the created address trees and assert that we only added one field to the "base", and all of them are distinct.
I am not sure it is worth the amount of code required. Once #58312 is finished, we will have asserts in VN that would have caught this case.
There was a problem hiding this comment.
I'm not a fan of how we handle the zero offset case today. We've had a number of bugs in this area.
Ok by me if it is too hard to check -- just wanted to see if you had any ideas.
|
|
||
| using System.Runtime.CompilerServices; | ||
|
|
||
| class FldSeqsInPromotedCpBlk |
There was a problem hiding this comment.
Can you add a comment linking this test to the PR and/or describing the problem this test is supposed to cover?
There was a problem hiding this comment.
Added one, hopefully it captures the problem precisely enough.
|
Native AOT build (a newly added thing, see #62833) is failing. I'm not familiar with this yet, so can't say what's up. It is running tests during the build step which means test failures don't get summarized the same way as for the other CI legs, so I have no idea if this failure is happening elsewhere too. cc @MichalStrehovsky @dotnet/jit-contrib |
|
Looks like the FinalizeTest also recently failed in runtimelab: dotnet/runtimelab#1702 (comment). The failure is probably unrelated. I've submitted #62924 to increase the reliability of the test. |
| { | ||
| // Use the orginal m_dstAddr tree when i == 0 | ||
| // Reuse the orginal m_dstAddr tree for the last field. |
There was a problem hiding this comment.
I'm not a fan of how we handle the zero offset case today. We've had a number of bugs in this area.
Ok by me if it is too hard to check -- just wanted to see if you had any ideas.
Reusing the address for the first field is problematic as the first field is likely to be at a zero offset, thus a zero-offset field sequence will be attached to it, and subsequent copies of the same tree will pick it up, which is incorrect and can result in silent bad codegen.
Fix by reusing the address for the last field instead.
Part of #58312.
Just one method with diffs: "better" sequences unblocked quite a few CSEs.