Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

tomeksowi
Copy link

@tomeksowi tomeksowi commented Jun 17, 2024

@tomeksowi tomeksowi changed the title Classify stack arguments as single segment [RISC-V] Classify stack arguments as single segment Jun 17, 2024
@jakobbotsch
Copy link
Owner

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
Copy link
Owner

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)).

@tomeksowi 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
Copy link
Author

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 tomeksowi marked this pull request as ready for review June 18, 2024 11:18
Comment on lines +2129 to +2155
#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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@LuckyXu-HF LuckyXu-HF Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jakobbotsch
Copy link
Owner

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.

@jakobbotsch jakobbotsch merged commit 9348fd1 into jakobbotsch:call-args-new-abi Jun 20, 2024
jakobbotsch pushed a commit that referenced this pull request Oct 4, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants