[RISC-V] Fix struct types passed by floating-point calling convention #4

tomeksowi · 2024-06-17T12:41:37Z

Part of dotnet#84834, cc @dotnet/samsung

jakobbotsch · 2024-06-17T20:30:24Z

Thanks for looking at this. Feel free to submit it upstream since it looks like it doesn't interact with any of the things I was doing in this branch.

jakobbotsch · 2024-06-18T06:18:09Z

I still need to resolve one more assert failure, seemingly unrelated to the stack arg classification (during crossgen2 of System.Numerics.Matrix3x2:setItem(int, int, float):this):
https://github.com/dotnet/runtime/blob/eb455ec34c6709e487c19e52c29ec712a6fa4d7f/src/coreclr/jit/lsrabuild.cpp#L606-L611

On a related note I think this assert can/should be tightened back to the same as all other targets; it should be possible after the dotnet#97368 (related comment: dotnet#97368 (comment)).

…ting-point calling convention

…atforms because after dotnet#97368 we don't need an exception

tomeksowi · 2024-06-18T11:18:01Z

Should work now, albeit platform-specific fix-ups are not the nicest thing. I phrased it utilizing new ABI passing info to make it easier when the old ones will eventually go.

tomeksowi · 2024-06-18T11:24:44Z

src/coreclr/jit/morph.cpp

+#if defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+            if (arg.NewAbiInfo.HasAnyFloatingRegisterSegment())
+            {
+                // Struct passed according to hardware floating-point calling convention
+                assert(arg.NewAbiInfo.NumSegments <= 2);
+                assert(!arg.NewAbiInfo.HasAnyStackSegment());
+                if (arg.NewAbiInfo.NumSegments == 2)
+                {
+                    // On LoongArch64, "getPrimitiveTypeForStruct" will incorrectly return "TYP_LONG"
+                    // for "struct { float, float }", and retyping to a primitive here will cause the
+                    // multi-reg morphing to not kick in (the struct in question needs to be passed in
+                    // two FP registers). Here is just keep "structBaseType" as "TYP_STRUCT".
+                    // TODO-LoongArch64: fix "getPrimitiveTypeForStruct".
+                    structBaseType = TYP_STRUCT;
+                }
+                else
+                {
+                    assert(arg.NewAbiInfo.NumSegments == 1);
+                    structBaseType = arg.NewAbiInfo.Segments[0].GetRegisterType();
+                }
+
+                for (unsigned i = 0; i < arg.NewAbiInfo.NumSegments; ++i)
+                {
+                    arg.AbiInfo.StructFloatFieldType[i] = arg.NewAbiInfo.Segments[i].GetRegisterType();
+                }
+            }
+#endif // defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)


cc @LuckyXu-HF @shushanhf

LGTM, thanks. I think we need some test further on LA64 after the relate refactor of these.

jakobbotsch · 2024-06-18T11:33:46Z

Should work now, albeit platform-specific fix-ups are not the nicest thing. I phrased it utilizing new ABI passing info to make it easier when the old ones will eventually go.

Looks good to me. I think this platform-specific fixup will disappear once the multireg morphing code is adapted to use the new ABI representation directly.

* bug #1: don't allow for values out of the SerializationRecordType enum range * bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug #3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug #4: document the fact that IOException can be thrown * bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum * bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)

* [NRBF] Don't use Unsafe.As when decoding DateTime(s) (dotnet#105749) * Add NrbfDecoder Fuzzer (dotnet#107385) * [NRBF] Fix bugs discovered by the fuzzer (dotnet#107368) * bug #1: don't allow for values out of the SerializationRecordType enum range * bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug #3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug #4: document the fact that IOException can be thrown * bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum * bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown) # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs * [NRBF] throw SerializationException when a surrogate character is read (dotnet#107532) (so far an ArgumentException was thrown) * [NRBF] Fuzzing non-seekable stream input (dotnet#107605) * [NRBF] More bug fixes (dotnet#107682) - Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay * [NRBF] Comments and bug fixes from internal code review (dotnet#107735) * copy comments and asserts from Levis internal code review * apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future * add missing and fix some of the existing comments * first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument * second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*]) * third bug fix: don't cast bytes to booleans * fourth bug fix: don't cast bytes to DateTimes * add one test case that I've forgot in previous PR # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs * [NRBF] Address issues discovered by Threat Model (dotnet#106629) * introduce ArrayRecord.FlattenedLength * do not include invalid Type or Assembly names in the exception messages, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack. * It is possible to have binary array records have an element type of array without being marked as jagged --------- Co-authored-by: Buyaa Namnan <[email protected]>

Classify stack arguments as single segment

d286b61

tomeksowi changed the title ~~Classify stack arguments as single segment~~ [RISC-V] Classify stack arguments as single segment Jun 17, 2024

tomeksowi mentioned this pull request Jun 18, 2024

[RISC-V] Classify stack arguments as single segment dotnet/runtime#103619

Merged

tomeksowi added 2 commits June 18, 2024 12:25

Bring back type fix-ups for structs passed according to hardware floa…

019622c

…ting-point calling convention

Use more strict reg masking condition for fixed refs like on other pl…

08390e5

…atforms because after dotnet#97368 we don't need an exception

tomeksowi changed the title ~~[RISC-V] Classify stack arguments as single segment~~ [RISC-V] Fix struct types passed by floating-point calling convention Jun 18, 2024

tomeksowi marked this pull request as ready for review June 18, 2024 11:18

tomeksowi commented Jun 18, 2024

View reviewed changes

jakobbotsch merged commit 9348fd1 into jakobbotsch:call-args-new-abi Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] Fix struct types passed by floating-point calling convention #4

[RISC-V] Fix struct types passed by floating-point calling convention #4

tomeksowi commented Jun 17, 2024 •

edited

Loading

jakobbotsch commented Jun 17, 2024

jakobbotsch commented Jun 18, 2024

tomeksowi commented Jun 18, 2024

tomeksowi Jun 18, 2024

LuckyXu-HF Jun 19, 2024 •

edited

Loading

jakobbotsch commented Jun 18, 2024

[RISC-V] Fix struct types passed by floating-point calling convention #4

[RISC-V] Fix struct types passed by floating-point calling convention #4

Conversation

tomeksowi commented Jun 17, 2024 • edited Loading

jakobbotsch commented Jun 17, 2024

jakobbotsch commented Jun 18, 2024

tomeksowi commented Jun 18, 2024

tomeksowi Jun 18, 2024

Choose a reason for hiding this comment

LuckyXu-HF Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

jakobbotsch commented Jun 18, 2024

tomeksowi commented Jun 17, 2024 •

edited

Loading

LuckyXu-HF Jun 19, 2024 •

edited

Loading