Fold primitive-typed access to promoted structs in local morph and forbid mismatched struct assignments #76766
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead, create constructions like "OBJ(ADDR(LCL_VAR))" that block morphing can recognize on its own.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsTwo changes:
As of this change, it will be the case that the two sides of a struct assignment will always agree on types, for which a special asset has been added. This will render most of We are expecting overall good diffs, most of the regressions are associated with more substitutions. There are also two cases in test code where the new logic is more conservative for
|
Closed
SingleAccretion
changed the title
SingleAccretion
force-pushed
the
LclMorph-Promotion-Simple-Upstream
branch
from
5ff9b77
to
24f8a34
Compare
And simplify the code...
Without handle equality, the assert no longer holds, for example:
```cs
private StructWithInt Problem(StructWithInt b, StructWithInt a)
{
a = ((StructWithStructWithInt*)&b)->StructWithInt;
return a;
}
```
Dead / unncessary. No diffs.
This is significantly simpler than moving the promotion logic to post-order because we don't need to fiddle with the ref counting process and complexities of intermediate states, e. g. "IND<float>(ADDR(FIELD<int>(ADDR(LCL_VAR<struct>))))" can be naturally turned into "BITCAST<float>(LCL_VAR<int>)" like this.
With this, we have only a very small regression: Base: 1297463478, Diff: 1297750893, +0.0222% LocalAddressVisitor::MorphStructField : 345099 : NA : 31.52% : +0.0266% LocalAddressVisitor::PreOrderVisit : 224684 : +3.53% : 20.52% : +0.0173% memset : 88591 : +0.99% : 8.09% : +0.0068% Compiler::fgMorphSmpOp : -19170 : -0.06% : 1.75% : -0.0015% Compiler::fgMorphStructField : -367474 : -100.00% : 33.56% : -0.0283% (This is with a dummy field on LclVarDsc) Losing the assert does not seem worrysome as we have an identical one in "fgMorphField".
SingleAccretion
force-pushed
the
LclMorph-Promotion-Simple-Upstream
branch
from
24f8a34
to
1e86541
Compare
|
@dotnet/jit-contrib |
|
@jakobbotsch can you take a look at this one? |
|
Nice cleanup, thanks again! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two changes:
ASG(struct, primitive)infgMorphStructField. This leads to some improvements (as well as regressions) due to us now being able to forward-substitute more things. It also decouples the replacement logic fromlvFieldHnd, making it unnecessary. This shrinks the size ofLclVarDscfrom 104 to 96 bytes on x64, resulting in~0.35%memory consumption reduction (for optimized code), and a TP loss because more instructions are used by MSVC to perform pointer arithmetic onLclVarDsc*s.fgMorphStructFieldto work withIND-based access. The alternative of moving the whole logic to post-order was explored, but ultimately discarded due to it being more complex.As of this change, it will be the case that the two sides of a struct assignment will always agree on types, for which a special assert has been added. This will render most of
fgMorphBlockOperandunnecessary, but deleting will have to wait till #76745 is in.We are expecting overall good diffs, most of the regressions are associated with more substitutions. There are also two cases in test code where the new logic is more conservative for
RETURNs, but it does not seem worth the code to fix them (by propagating the quirkedRETURNhandling into local morph).