JIT: Remove TYP_BLK and TYP_LCLBLK #83036
Conversation
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis PR allows TYP_STRUCT locals to have block layouts and replaces uses This is a precursor to #83005. I hit several cases where we are unable to create a proper local for a tree we wish to extract. This unification should greatly simplify those cases.
|
jakobbotsch
changed the title
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Need to look at TP regressions. One thing I noticed when I was debugging a bug before is that the change causes us to sometimes newly set |
|
@TIHan I have a SuperFileCheck error now because runtime/src/tests/JIT/opt/Add/IntAdd.cs Line 16 in b6fc7bd Is matching the "add" in "addr-exposed" here: Can we do something to make this less fragile? E.g. make sure SuperFileCheck starts matching from the actual instructions. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| @@ -988,7 +988,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor> | |||
| { | |||
| isWide = endOffset.Value() > m_compiler->lvaLclExactSize(lclNum); | |||
|
|
|||
| if (varDsc->TypeGet() == TYP_BLK) | |||
| if ((varDsc->TypeGet() == TYP_STRUCT) && varDsc->GetLayout()->IsBlockLayout()) | |||
There was a problem hiding this comment.
@SingleAccretion Do you think removing this requires any downstream changes now that these are TYP_STRUCT?
I'll probably try that in a follow-up.
There was a problem hiding this comment.
Do you think removing this requires any downstream changes now that these are
TYP_STRUCT?
I wouldn't expect any.
I think that looks fine. In the future, I think I would like to have an output that looks like what Disasmo does. |
This PR allows TYP_STRUCT locals to have block layouts and replaces uses of TYP_BLK and TYP_LCLBLK with such locals instead.
Otherwise we may track it which is a TP regression.
|
A large part of the linux-x64 and win-x64 MinOpts regressions are more instances of MSVC emitting more instructions due to changed field offsets/struct sizes. For example, in mov ecx, r8d
- lea rdx, [rcx+rcx*2]
- add rdx, rdx
- mov rax, [rsi+rdx*8+130h]
+ lea rdx, [rcx+6]
+ lea rdx, [rdx+rdx*2]
+ add rdx, rdx
+ mov rax, [rsi+rdx*8]
mov dword ptr [rsi+rcx*4+0E6Ch], 0FFFFFFFFhIn the base the code needs to compute If I just readd the types back to typelist.h, but change nothing else, then the TP diff for linux-x64 looks like the following. Still a bit to investigate here. Overall (-0.10% to -0.06%)
MinOpts (-0.08% to +0.10%)
FullOpts (-0.12% to -0.06%)
DetailsAll contexts:
MinOpts contexts:
FullOpts contexts:
|
This reverts commit 4e2f9ab.
jakobbotsch
force-pushed
the
remove-TYP_BLK-TYP_LCLBLK
branch
from
de8729d
to
ee37405
Compare
|
To get rid of the last tier-0 TP regressions I have added a fast-path for the 0-sized block layout in linux-x64Overall (-0.11% to -0.09%)
MinOpts (-0.09% to +0.01%)
FullOpts (-0.13% to -0.09%)
DetailsAll contexts:
MinOpts contexts:
FullOpts contexts:
win-x64Overall (-0.10% to -0.07%)
MinOpts (-0.08% to +0.09%)
FullOpts (-0.12% to -0.07%)
DetailsAll contexts:
MinOpts contexts:
FullOpts contexts:
Note that libraries.crossgen2 has only 15 MinOpts contexts, so I don't think it is a representative sample. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
The fuzzlyn failures repro on main too (one of them is #83140, one of the others look like a similar related issue, the last one doesn't). I'll open issues for them. Diffs. See #83036 (comment) for the explanation on the linux-x64 and win-x64 MinOpts TP regressions. cc @dotnet/jit-contrib PTAL @SingleAccretion @EgorBo @AndyAyersMS |
| // JIT32 encoder cannot handle GS cookie at fp+0 since NO_GS_COOKIE == 0. | ||
| // Add some padding if it is the last allocated local. | ||
| if ((lvaGSSecurityCookie != BAD_VAR_NUM) && (lvaGetDesc(lvaGSSecurityCookie)->GetStackOffset() == stkOffs)) | ||
| { | ||
| lvaIncrementFrameSize(TARGET_POINTER_SIZE); | ||
| stkOffs -= TARGET_POINTER_SIZE; | ||
| } |
There was a problem hiding this comment.
I hit this in some cases where we create a local for a small constant-sized stackalloc. We emit GS cookie check for those, but now that the stackalloc local is a TYP_STRUCT we sometimes are able to eliminate it and not allocate any stack space for it, causing us to end up with the GS cookie at fp+0.
| #if FEATURE_FIXED_OUT_ARGS | ||
|
|
||
| /* Is this the dummy variable representing GT_LCLBLK ? */ | ||
| needSlot |= (lclNum == lvaOutgoingArgSpaceVar); | ||
|
|
||
| #endif // FEATURE_FIXED_OUT_ARGS | ||
|
|
There was a problem hiding this comment.
This is no longer necessary since lvaOutgoingArgSpaceVar is a normal address-exposed struct local now and handled above.
| case TYP_BYREF: | ||
| if (isZeroed) | ||
| { | ||
| // LclVars of TYP_BYREF can be zero-inited. | ||
| initVal = vnStore->VNForByrefCon(0); | ||
| } | ||
| else | ||
| { | ||
| // Here we have uninitialized TYP_BYREF | ||
| initVal = vnStore->VNForFunc(typ, VNF_InitVal, vnStore->VNForIntCon(lclNum)); | ||
| } | ||
| break; |
There was a problem hiding this comment.
The handling for this case seemed to be identical with the default case below, so we can remove the entire switch in favor of the default case.
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The GC stress failures are eventpipe test timeouts that are preexisting. |
This PR allows TYP_STRUCT locals to have block layouts and replaces uses
of TYP_BLK and TYP_LCLBLK with such locals instead.
There is still an invariant that any struct parameter local (even SIMD) has a non-block layout.
This is a precursor to #83005. I hit several cases where we are unable to create a proper local for a tree we wish to extract. This unification should greatly simplify those cases.