Emit type layout check fixups #34927
Conversation
|
cc @dotnet/crossgen-contrib |
src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.Algorithm.cs
Outdated
Show resolved
Hide resolved
|
Do we also need to check that the struct gets the exact same ABI treatment? Eg changing from a struct of two ints to a struct of two floats we have same size / alignment / gc, but latter might get classified as an HFA and so be passed / returned differently. |
|
There is
|
nattress
force-pushed
the
TypeLayoutChecks
branch
from
ceafde7 to
fca906a
Compare
| flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty; | ||
| } | ||
|
|
||
| bool isHfa = defType.IsHfa && defType.Context.Target.Architecture == TargetArchitecture.ARM || defType.Context.Target.Architecture == TargetArchitecture.ARM64; |
There was a problem hiding this comment.
Should IsHfa rather return false except on ARM and ARM64 ? It does not sounds right to special-case it at callsites like this.
There was a problem hiding this comment.
I moved it into the Hfa computation algorithm
| flags |= ReadyToRunTypeLayoutFlags.READYTORUN_LAYOUT_GCLayout_Empty; | ||
| } | ||
|
|
||
| bool isHfa = defType.IsHfa && defType.Context.Target.Architecture == TargetArchitecture.ARM || defType.Context.Target.Architecture == TargetArchitecture.ARM64; |
There was a problem hiding this comment.
The IsHFA computation in crossgen2 looks slightly different from what CoreCLR does.
E.g. we might need to change the implementation of ComputeValueTypeShapeCharacteristics on VectorFieldLayoutAlgorithm to account for their special HFA status.
There was a problem hiding this comment.
(But it looks like a pre-existing problem that we should just track somewhere)
There was a problem hiding this comment.
The IsHFA computation in crossgen2 looks slightly different from what CoreCLR does.
E.g. we might need to change the implementation of
ComputeValueTypeShapeCharacteristicsonVectorFieldLayoutAlgorithmto account for their special HFA status.
I have implemented that. Need to merge with this PR.
...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Outdated
Show resolved
Hide resolved
...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Outdated
Show resolved
Hide resolved
...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Show resolved
Hide resolved
...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Outdated
Show resolved
Hide resolved
...crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs
Outdated
Show resolved
Hide resolved
| public partial struct GCPointerMap : IEquatable<GCPointerMap>, IComparable<GCPointerMap> | ||
| { | ||
| // Each bit in this array represents a pointer-sized cell. | ||
| private int[] _gcFlags; |
There was a problem hiding this comment.
Nit - uint might better convey the fact that this is bitmask.
There was a problem hiding this comment.
I ported this from CoreRT. I can see if we can quickly switch over to uints instead, sure.
There was a problem hiding this comment.
Please don't spend too much time on it, if it turns out to be viral for whatever reason, there's just no reason to waste time on it. Thanks for taking a look nonetheless.
There was a problem hiding this comment.
It wasn't that viral - I adjusted them to be uints.
src/coreclr/src/tools/Common/TypeSystem/Common/Utilities/GCPointerMap.cs
Show resolved
Hide resolved
When a ready-to-run compiled method uses value types from other version bubbles, emit a precode fixup to check at runtime that the type is still compatible (same size, alignment, GC layout). Bring over `GCPointerMap*.cs` from CoreRT repo to get the compute the GC layout bytes that match what the runtime expects in the layout blob. Simplify the behavior of the Crossgen test scripting. If tests opt out of crossgen by setting `<CrossGenTest>false</CrossGenTest>`, skip emitting the crossgen commands into the test run script. Also, stop setting the `RunCrossGen` and `RunCrossGen2` environment variables in the test script based on compile-time settings. These get set by runtest.py anyway. This set of tweaks allow for crossgen tests which we don't want to run the default crossgen commands on, and allow choosing either crossgen or crossgen2 as part of test execution.
* Move architecture filtering for Hfas to `MetadataFieldLayoutAlgorithm` * Switch `GCPointerMap` to use `uint` for flags instead of `int`
nattress
force-pushed
the
TypeLayoutChecks
branch
from
3be0960 to
08eae55
Compare
ghost
locked as resolved and limited conversation to collaborators
When a ready-to-run compiled method uses value types from other version bubbles, emit a precode fixup to check at runtime that the type is still compatible (same size, alignment, GC layout).
Bring over
GCPointerMap*.csfrom CoreRT repo to get the compute the GC layout bytes that match what the runtime expects in the layout blob.Simplify the behavior of the Crossgen test scripting. If tests opt out of crossgen by setting
<CrossGenTest>false</CrossGenTest>, skip emitting the crossgen commands into the test run script. Also, stop setting theRunCrossGenandRunCrossGen2environment variables in the test script based on compile-time settings. These get set by runtest.py anyway. This set of tweaks allow for crossgen tests which we don't want to run the default crossgen commands on, and allow choosing either crossgen or crossgen2 as part of test execution.