JIT: Check more invariants on ABI info and fix some arm32 bugs #101372

jakobbotsch · 2024-04-22T09:23:43Z

Implicit byref parameters should be passed as a single pointer sized segment
Segments should not point outside the local size
Segments should not overlap each other
Segments should have size > 0
Segments should be ordered by offset
Fix a bug in the arm32 classifier when structs are split
Fix a bug in the arm32 classifier for odd-sized structs with 8 byte alignment. For example

[StructLayout(LayoutKind.Sequential, Size = 12)]
struct S
{
    public double X;
    public float Y;
}

would be considered to take 4 slots before.

Fix a bug in the Swift classifier that would cause the tail segments to have an out-of-bounds size

- Implicit byref parameters should be passed as a single pointer sized segment - Segments should not point outside the local size - Segments should not overlap each other

dotnet-policy-service · 2024-04-22T09:24:16Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

shushanhf · 2024-04-22T09:47:26Z

src/coreclr/jit/lclvars.cpp

+            {
+                const ABIPassingSegment& segment = abiInfo.Segments[i];
+                assert(segment.Size > 0);
+                assert(segment.Offset + segment.Size <= lvaLclExactSize(lclNum));


The old ABI-style used LclVarDsc::lvSize() to get some struct's size as the LclVarDsc::lvExactSize() is not always we expected size.

lvaLclExactSize is the correct size to use. lvSize is what is allocated by the backend on the stack frame, but this is not the real size of the struct being passed.

I want the ABI information to contain the real size to be as precise as possible. If the backend wants it can round this size up when stores/loads are generated by using the knowledge about how it allocates more space in some cases.

What error did you run into around this?

#101365 (comment)

For the V02 arg2 [V02 ] ( 1, 1 ) struct ( 8) [fp-0x18] do-not-enreg[S] System.Threading.Tasks.VoidTaskResult>, the size is 1 which returned by lvaLclExactSize(lclNum) or GetLayout()->GetSize(), but the old ABI-style is 8.

/// new-ABI-style: { Start Jitting Method 615 System.Threading.Tasks.Task`1[System.Threading.Tasks.VoidTaskResult]:.ctor(ubyte,System.Threading.Tasks.VoidTaskResult,int,System.Threading.CancellationToken):this (MethodHash=05f08360) MinOpts ; Assembly listing for method System.Threading.Tasks.Task`1[System.Threading.Tasks.VoidTaskResult]:.ctor(ubyte,System.Threading.Tasks.VoidTaskResult,int,System.Threading.CancellationToken):this (MinOpts) ; Emitting BLENDED_CODE for generic LOONGARCH64 - Unix ; MinOpts code ; debuggable code ; fp based frame ; fully interruptible ; No PGO data ; Final local variable assignments ; ; V00 this [V00 ] ( 1, 1 ) ref -> [fp-0x08] do-not-enreg[] this class-hnd <System.Threading.Tasks.Task`1[System.Threading.Tasks.VoidTaskResult]> ; V01 arg1 [V01 ] ( 1, 1 ) ubyte -> [fp-0x0C] do-not-enreg[] ; V02 arg2 [V02 ] ( 1, 1 ) struct ( 8) [fp-0x18] do-not-enreg[S] <System.Threading.Tasks.VoidTaskResult> ; V03 arg3 [V03 ] ( 1, 1 ) int -> [fp-0x1C] do-not-enreg[] ; V04 arg4 [V04 ] ( 1, 1 ) struct ( 8) [fp-0x28] do-not-enreg[S] <System.Threading.CancellationToken> ; V05 loc0 [V05 ] ( 1, 1 ) ubyte -> [fp-0x2C] do-not-enreg[] ;# V06 OutArgs [V06 ] ( 1, 1 ) struct ( 0) [sp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace" ; ; Lcl frame size = 48 Frame info. #outsz=0; #framesz=64; lcl=48 G_M31903_IG01: ;; offset=0x0000 0xff76b50e98 02FF0063 addi.d sp, sp, -64 0xff76b50e9c 29C0C076 st.d fp, sp, 48 0xff76b50ea0 29C0E061 st.d ra, sp, 56 0xff76b50ea4 02C0C076 addi.d fp, sp, 48 0xff76b50ea8 29FFE2C4 st.d a0, fp, -8 0xff76b50eac 29BFD2C5 st.w a1, fp, -12 0xff76b50eb0 29BFA2C6 st.w a2, fp, -24 //error. 0xff76b50eb4 29BF92C7 st.w a3, fp, -28 0xff76b50eb8 29FF62C8 st.d a4, fp, -40 ;; size=36 bbWeight=1 PerfScore 0.00

/// old-ABI-style: G_M31903_IG01: ;; offset=0x0000 0xff74170e98 02FF0063 addi.d sp, sp, -64 0xff74170e9c 29C0C076 st.d fp, sp, 48 0xff74170ea0 29C0E061 st.d ra, sp, 56 0xff74170ea4 02C0C076 addi.d fp, sp, 48 0xff74170ea8 29FFE2C4 st.d a0, fp, -8 0xff74170eac 29BFD2C5 st.w a1, fp, -12 0xff74170eb0 29FFA2C6 st.d a2, fp, -24 /// this is ok and we expected. 0xff74170eb4 29BF92C7 st.w a3, fp, -28 0xff74170eb8 29FF62C8 st.d a4, fp, -40

Why is the 4 byte store an error when the struct is only 1 byte in size?

FWIW, according to https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html

Bits unused due to padding, and bits past the end of a structure whose size in bits is not divisible by GRLEN, are undefined.

IIUC the upper 7 bytes of a2 are undefined. So either st.d or st.w leave the 7 extra bytes allocated for V02 as undefined.

I just diff the new-style ABI with the old-style ABI, then I just recover the old-style's instruction.
I had deleted the roundup of struct size.

Thanks

The previous code would consider a struct like ```csharp [StructLayout(LayoutKind.Sequential, Size = 12)] struct S { public double X; public float Y; } ``` to take 4 slots, which is wrong.

jakobbotsch · 2024-04-24T08:37:53Z

cc @dotnet/jit-contrib PTAL @EgorBo

jakobbotsch · 2024-04-25T20:10:20Z

@AndyAyersMS Can you take a look since Egor is out?

…t#101372) - Implicit byref parameters should be passed as a single pointer sized segment - Segments should not point outside the local size - Segments should not overlap each other - Segments should have size > 0 - Segments should be ordered by offset - Fix a bug in the arm32 classifier when structs are split - Fix a bug in the arm32 classifier for odd-sized structs with 8 byte alignment. For example ```csharp [StructLayout(LayoutKind.Sequential, Size = 12)] struct S { public double X; public float Y; } ``` would be considered to take 4 slots before. - Fix a bug in the Swift classifier that would cause the tail segments to have an out-of-bounds size

JIT: Check more invariants on ABI info

0364ad0

- Implicit byref parameters should be passed as a single pointer sized segment - Segments should not point outside the local size - Segments should not overlap each other

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 22, 2024

dotnet-policy-service bot assigned jakobbotsch Apr 22, 2024

jakobbotsch mentioned this pull request Apr 22, 2024

[LoongArch64] amend the LoongArch64Classifier::Classify for passing struct. #101365

Merged

Nonzero size and sorted by offset

d354f7f

shushanhf reviewed Apr 22, 2024

View reviewed changes

jakobbotsch added 2 commits April 22, 2024 12:28

Fix arm32 stack segment offset

bd3f866

Avoid rounding size up to alignment on arm32

fbead21

The previous code would consider a struct like ```csharp [StructLayout(LayoutKind.Sequential, Size = 12)] struct S { public double X; public float Y; } ``` to take 4 slots, which is wrong.

build-analysis bot mentioned this pull request Apr 22, 2024

System.Net.Tests.HttpWebRequestTest_Async.GetResponseAsync_ParametersAreNotCachable_CreateNewClient test fails #100912

Closed

Adjust Swift tails

5effcc8

This was referenced Apr 22, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

jakobbotsch marked this pull request as ready for review April 24, 2024 08:37

jakobbotsch changed the title ~~JIT: Check more invariants on ABI info~~ JIT: Check more invariants on ABI info and fix some arm32 bugs Apr 24, 2024

jakobbotsch requested a review from EgorBo April 24, 2024 08:37

jakobbotsch mentioned this pull request Apr 25, 2024

[RISC-V] Use common CodeGen::genHomeRegisterParams #101288

Merged

jakobbotsch requested a review from AndyAyersMS April 25, 2024 20:10

AndyAyersMS approved these changes Apr 25, 2024

View reviewed changes

jakobbotsch merged commit 99dd60d into dotnet:main Apr 26, 2024
106 of 110 checks passed

jakobbotsch deleted the value-size-invariant branch April 26, 2024 08:00

github-actions bot locked and limited conversation to collaborators May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Check more invariants on ABI info and fix some arm32 bugs #101372

JIT: Check more invariants on ABI info and fix some arm32 bugs #101372

jakobbotsch commented Apr 22, 2024 •

edited

Loading

dotnet-policy-service bot commented Apr 22, 2024

shushanhf Apr 22, 2024

jakobbotsch Apr 22, 2024

shushanhf Apr 22, 2024 •

edited

Loading

jakobbotsch Apr 22, 2024

jakobbotsch Apr 22, 2024

shushanhf Apr 23, 2024

jakobbotsch commented Apr 24, 2024

jakobbotsch commented Apr 25, 2024

JIT: Check more invariants on ABI info and fix some arm32 bugs #101372

JIT: Check more invariants on ABI info and fix some arm32 bugs #101372

Conversation

jakobbotsch commented Apr 22, 2024 • edited Loading

dotnet-policy-service bot commented Apr 22, 2024

shushanhf Apr 22, 2024

Choose a reason for hiding this comment

jakobbotsch Apr 22, 2024

Choose a reason for hiding this comment

shushanhf Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

jakobbotsch Apr 22, 2024

Choose a reason for hiding this comment

jakobbotsch Apr 22, 2024

Choose a reason for hiding this comment

shushanhf Apr 23, 2024

Choose a reason for hiding this comment

jakobbotsch commented Apr 24, 2024

jakobbotsch commented Apr 25, 2024

jakobbotsch commented Apr 22, 2024 •

edited

Loading

shushanhf Apr 22, 2024 •

edited

Loading