Make it possible to align at 32 byte boundaries#6675
Make it possible to align at 32 byte boundaries#6675MichalStrehovsky merged 3 commits intodotnet:masterfrom
Conversation
|
Is this actually fixing the alignment or just fixing up the struct packing for these fields? |
| if (Abi == TargetAbi.ProjectN) | ||
| { | ||
| // ProjectN doesn't support hardware intrinsics | ||
| return 8; |
There was a problem hiding this comment.
Why special-case this for ProjectN? The layout of the struct should be consistent (IMO), even if you don't support codegen for any of the types. This allows the Vector64<T>, Vector128<T>, and Vector256<T> types to still be used as the corresponding ABI type for interchange/interop scenarios.
There was a problem hiding this comment.
Bumping this value up has negative effects on universal shared code (RyuJIT doesn't concern itself with it, but it's a thing on Project N).
This constant is used in cases like this:
class Foo<T>
{
T myField;
}When T is __UniversalCanon (could be a ref type, could be a struct, we have no clue): with maximum supported alignment of 8, we can still assign an offset to myField because the field is already aligned at the maximum possible alignment (the previous type ends at offset 8), no matter what T ends up being at runtime.
Once this is bumped to 32, T could be something that requires alignment of 16 or 32 and we can no longer give it an offset (so accessing the field from generated code becomes: go call a helper to find offset of myField). This is a net perf loss. No point in paying it when the backend is UTC.
There was a problem hiding this comment.
Will ProjectN never support hardware intrinsics? Because it will be a breaking change to modify this in the future if users have a T that is a Vector128<T> otherwise (the layout of the struct will change, which will impact unsafe code, offsets, etc).
There was a problem hiding this comment.
This constant is used in cases like this:
class Foo<T>
{
T myField;
}
What happens in CoreCLR in cases like these today?
For reference types, Vector64/128/256 alignment requirements should be irrelevant since the GC will allocate the whole type with an arbitrary offsets anyway.
For struct types, where does this alignment matter? Is it just that the offsets are aligned; or does the JIT actually allocate these types on aligned addresses on the stack?
There was a problem hiding this comment.
What happens in CoreCLR in cases like these today?
CoreCLR appropriately packs T according to the packing for T. We have tests covering this here: https://github.com/dotnet/coreclr/blob/master/tests/src/Interop/StructPacking/StructPacking.cs#L27
There was a problem hiding this comment.
For struct types, where does this alignment matter? Is it just that the offsets are aligned; or does the JIT actually allocate these types on aligned addresses on the stack?
The alignment technically matters for all cases. I believe that, for Vector128<T> we generally work (since stack alignment defaults to 16), but I don't think we have anything special that guarantees this for stack allocations today (we probably should for both this and RVA statics, since those should be possible to make "pay to play").
This just bumps the number for maximum supported alignment from 8 to a higher number. |
Sorry, I'm still not certain what this means. I'm trying to determine if this just matches the CoreCLR behavior (structures will be packed appropriately and fields will be at the correct/expected offsets; but structures may not be allocated at the "correct" address), or if it also fixes the alignment of these structs when allocated (for a |
| /// Gets the maximum alignment to which something can be aligned | ||
| /// </summary> | ||
| public static int MaximumAlignment | ||
| public int MaximumAlignment |
There was a problem hiding this comment.
Looking at this now, maybe MaximumAlignment should actually be moved to the type system context if we decide to go the instance field route.
There was a problem hiding this comment.
MaximumAlignment is an optimization for universal shared generic code. The moment it becomes too big like what we are doing here it becomes useless. We can just drop it completely.
This doesn't do anything for Vector64/128/256 - it's not getting special cased anywhere. This pull request just opens the opportunity to have something with alignment higher than 8 in the system. |
Right. So my question is: When we do actually add types that have higher alignments (such as special-casing the HWIntrinsic types) will it actually impact the allocation alignment or just the struct packing? |
CPAOT compiler (that this change is mostly for) generates ready to run code to run on top of CoreCLR. Do you expect we would need to do something for allocation alignment here? My expectation is that the ready to run helper generated by the runtime will deal with that. Struct packing for vectors will be dealt with after we settle on a design for this max alignment extensibility point. (If and when we do something for hardware intrinsic on the CoreRT runtime and the flavor of the compiler that targets that runtime is up in the air. It will quite likely have to be one of my weekend projects.) |
|
The ABI requirements for On CoreCLR, we currently respect the packing (which ensures layout is correct) but do not respect the alignment requirement (due to limitations with the GC, and the fact that adding the support is not currently thought to be easily possible in a "pay to play" manner). I think for the "ready to run" scenario, just ensuring that the layout is correct would be sufficient. I think we also need to ensure the layout is correct for ProjectN (otherwise you end up in a scenario where ProjectN vs everything else has a different struct layout). I also think that we should be special-casing the layout of |
We may want to have issue opened for this so that it is not forgotten once/if these Vector types ship in ProjectN. |
davidwrighton
left a comment
There was a problem hiding this comment.
This change looks good. The discussion around ProjectN will need to be done as part of .NET Native, and not as part of the CoreRT repo especially as we'll need to update the underlying compiler to be aware of this sort of change as well.
This will allow us to do layout of types that have `Vector256` in them. I'm making this conditional and not enabled in Project N for now, since the Project N code generator doesn't support the hardware intrinsics anyway and increasing this value means that we have fewer places where we know field placement when universal shared generics are involved.
xUnit sniffs into properties otherwise and we assert/throw in some.
47bacbd to
376dc3a
Compare
|
I've rebased this against latest to get rid of a merge conflict in a R2R file. I'm keeping the no merge flag because I would like to do an integration to nmirror first. I'm expecting some ToF failure fun from all the changes that accumulated over time and would like to limit the delta. |
This will be necessary to support computing layout for types that embed
Vector256.Follow up on #6663 (comment).
This is foundational work needed to support the new HW intrinsics. We may or may not end up implementing function multiversioning for ready to run for .NET Core 3.0 CPAOT, but being able to compute the layout will let us at least pregenerate method bodies that pass through vector types without actually calling the intrinsics.
See individual commits. I'm flexible on whether we should include 47bacbd.