[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 <bunamnan@microsoft.com>

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

Comments

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

[RISC-V] Fix struct types passed by floating-point calling convention#4
jakobbotsch merged 3 commits intojakobbotsch:call-args-new-abifrom
tomeksowi:single-stack-arg-classifier

tomeksowi commented Jun 17, 2024 •

edited

Loading

Uh oh!

jakobbotsch commented Jun 17, 2024

Uh oh!

jakobbotsch commented Jun 18, 2024

Uh oh!

tomeksowi commented Jun 18, 2024

Uh oh!

tomeksowi Jun 18, 2024

Uh oh!

LuckyXu-HF Jun 19, 2024 •

edited

Loading

Uh oh!

jakobbotsch commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

tomeksowi commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch commented Jun 17, 2024

Uh oh!

jakobbotsch commented Jun 18, 2024

Uh oh!

tomeksowi commented Jun 18, 2024

Uh oh!

tomeksowi Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

LuckyXu-HF Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomeksowi commented Jun 17, 2024 •

edited

Loading

LuckyXu-HF Jun 19, 2024 •

edited

Loading