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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Apr 26, 2024
Merged

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

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0364ad0
JIT: Check more invariants on ABI info
jakobbotsch Apr 22, 2024
d354f7f
Nonzero size and sorted by offset
jakobbotsch Apr 22, 2024
bd3f866
Fix arm32 stack segment offset
jakobbotsch Apr 22, 2024
fbead21
Avoid rounding size up to alignment on arm32
jakobbotsch Apr 22, 2024
5effcc8
Adjust Swift tails
jakobbotsch Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/coreclr/jit/lclvars.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1885,6 +1885,42 @@ void Compiler::lvaClassifyParameterABI()
}
}
}

for (unsigned lclNum = 0; lclNum < info.compArgsCount; lclNum++)
{
const ABIPassingInformation& abiInfo = lvaGetParameterABIInfo(lclNum);

if (lvaIsImplicitByRefLocal(lclNum))
{
assert((abiInfo.NumSegments == 1) && (abiInfo.Segments[0].Size == TARGET_POINTER_SIZE));
}
else
{
for (unsigned i = 0; i < abiInfo.NumSegments; i++)
{
const ABIPassingSegment& segment = abiInfo.Segments[i];
assert(segment.Size > 0);
assert(segment.Offset + segment.Size <= lvaLclExactSize(lclNum));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@shushanhf shushanhf Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


if (i > 0)
{
assert(segment.Offset > abiInfo.Segments[i - 1].Offset);
}

for (unsigned j = 0; j < abiInfo.NumSegments; j++)
{
if (i == j)
{
continue;
}

const ABIPassingSegment& otherSegment = abiInfo.Segments[j];
assert((segment.Offset + segment.Size <= otherSegment.Offset) ||
(segment.Offset >= otherSegment.Offset + otherSegment.Size));
}
}
}
}
#endif // DEBUG
}

Expand Down
Loading