Uh oh!
There was an error while loading. Please reload this page.
[RISC-V] Simplify flags for passing struct in registers - #100080
Conversation
Asserts in `./Interop/StructMarshalling/PInvoke/MarshalStructAsLayoutExp/MarshalStructAsLayoutExp.sh` Error message is `Assertion failed 'roundUp(structSize, TARGET_POINTER_SIZE) == roundUp(loadExtent, TARGET_POINTER_SIZE)' in 'Managed:MarshalStructAsParam_AsExpByVal(int)' during 'Morph - Global' (IL size 2208; hash 0x9fd9734a; MinOpts)` Copied missed codes of GetRiscV64PassStructInRegiste in vm to crossgen2
c5b27c4 to
1d6f53aCompare…fake CorElementTypes from native branch
…convention tells us to
…C# versions of this method look more alike
…oop won't do any work if there's no fields
Alright, I think the bulk of work is done. CLR tests are looking ok. I also ran some tests with RunCrossGen2=1, interop- and ABI-related tests are passing and a full CLR test run with RunCrossGen2=1 is under way which will take some time. |
tomeksowi
commented
Mar 29, 2024
Too many crashes. The discrepancies in GetRISCV64PassStructInRegisterFlags I seen so far seem not to be the cause (new version returns better flags IMHO): I'll solve the crashes when I return from vacation. |
…() is null on empty structs, which crashes pFieldStart->GetFieldType()
tomeksowi
commented
Apr 9, 2024
CLR tests with RunCrossGen2=1 look good now. PR ready for review. |
tomeksowi
commented
Apr 9, 2024
I played around more with empty structs and they need some more work. Namely, we assume in quite a few places that only structs no bigger than 16 bytes can be passed in registers, which is not true for e.g. struct { double; struct {}; long; }. However, empty structs are rather niche so I'll go down that rabbit hole in another PR to make review of this one easier and sooner. |
tomeksowi
commented
Apr 17, 2024
@MichalStrehovsky@jkotas could you review please? |
| } | ||
| if ((numIntroducedFields == 0) || (numIntroducedFields > 2) || (typeDesc.GetElementSize().AsInt > 16)) | ||
| private static bool FlattenFieldTypes(TypeDesc td, StructFloatFieldInfoFlags[] types, ref int typeIndex) |
There was a problem hiding this comment.
| privatestaticboolFlattenFieldTypes(TypeDesctd,StructFloatFieldInfoFlags[]types,refinttypeIndex) | |
| privatestaticboolFlattenFieldTypes(TypeDesctd,Span<StructFloatFieldInfoFlags>types,refinttypeIndex) |
Can this use span and stackallocated space in the caller to avoid GC allocations?
There was a problem hiding this comment.
Sure.
There's still more opportunities, like use a single ref to struct { StructFloatFieldInfoFlags[2] types; int index; } (or even pass & return by value, it's so small). Since I'll be revisiting this in another PR to handle corner-cases with empty fields, I'll probably do sth like this in both C# and C++ while at it.
* [RISC-V] Fix struct info value in crossgen2 * [RISC-V] Fix assertion in crossgen Asserts in `./Interop/StructMarshalling/PInvoke/MarshalStructAsLayoutExp/MarshalStructAsLayoutExp.sh` Error message is `Assertion failed 'roundUp(structSize, TARGET_POINTER_SIZE) == roundUp(loadExtent, TARGET_POINTER_SIZE)' in 'Managed:MarshalStructAsParam_AsExpByVal(int)' during 'Morph - Global' (IL size 2208; hash 0x9fd9734a; MinOpts)` Copied missed codes of GetRiscV64PassStructInRegiste in vm to crossgen2 * Check size in GetRiscV64PassStructInRegisterFlags early, use named constant * Simplify managed branch of GetRiscV64PassStructInRegisterFlags * Fix assert IsPrimitiveType for 2nd field * Handle empty structs * Apply FIELD_SIZE_IS8 flags only when there's at least one float * Handle empty array struct elements * Enregister any field type <= 8 bytes, not just primitives; i.e. pointers and refs are also OK * Simplify native layout branch of GetRiscV64PassStructInRegisterFlags * Rewrite native branch to look at only at native layout info * Calculate flags already in GetFlattenedFieldTypes to avoid returning fake CorElementTypes from native branch * Ignore empty structs during field flattenting because RISC-V calling convention tells us to * Simplify crossgen2 GetRISCV64PassStructInRegisterFlags, make C++ and C# versions of this method look more alike * Remove early exit if nFields == 0 because it wasn't doing much, the loop won't do any work if there's no fields * Return early from HasImpliedRepeatedFields. GetApproxFieldDescListRaw() is null on empty structs, which crashes pFieldStart->GetFieldType() * Cleanup GetRiscV64PassStructInRegisterFlags call sites * Stackalloc field types to avoid GC allocations --------- Co-authored-by: Dong-Heon Jung <clamp03@gmail.com>
Discussion in #99602 pointed out that MethodTable::GetRiscV64PassStructInRegisterFlags grew too complicated to properly compare against its crossgen2 version in RISCV64PassStructInRegister.cs (and understand in general). This PR simplifies it (LOC count reduced by ~70%) by following the wording of RISC-V ABI more closely, in particular by introducing an explicit step for struct field hierarchy flattening which the ABI enregistration rules are based on.
As a result of the re-write, MethodTable::GetRiscV64PassStructInRegisterFlags started returning correct flags (pass in float+integer registers) for the two cases:
struct { float; struct { string; }; }- the wrapper struct for string (one pointer) shouldn't matterstruct { float; struct {/*empty*/}; int; }- empty struct fields should be ignoredRISCV64PassStructInRegister.GetRISCV64PassStructInRegisterFlags was also re-written to match MethodTable::GetRiscV64PassStructInRegisterFlags.
This PR also fixes assertions in fgMorphMultiregStructArg when crossgen2 compiles in Debug build with --verify-type-and-field-layout option. Error messages:
Part of #84834, cc @dotnet/samsung