JIT: Rewrite initial parameter frame layout in terms of new ABI info #101340

jakobbotsch · 2024-04-20T18:31:15Z

Rewrite lvaAssignVirtualFrameOffsetsToArgs to make use of the ABI information that was already computed as part of ABI classification in the frontend.

No diffs are expected.

Rewrite `lvaAssignVirtualFrameOffsetsToArgs` to make use of the ABI information that was already computed as part of ABI classification in the frontend.

dotnet-policy-service · 2024-04-20T18:31:52Z

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

Considering the stack alignment to be part of the passed stack size does not really make sense since the callee does not own the extra bytes added to ensure alignment.

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

cc @dotnet/jit-contrib PTAL @BruceForstall @kunalspathak

No diffs. Some minor TP improvements.

Also FYI @shushanhf @dotnet/samsung since this affects LA64/RISCV64 as well.

shushanhf · 2024-04-23T07:56:59Z

src/coreclr/jit/lclvars.cpp

-            //            Signature (int a0, int a1, int a2, struct {long} a3, ...)
+            // - On Windows, the Arm64 varargs ABI can split a 16 byte struct across x7 and stack
+            // - Arm32 generally allows structs to be split
+            // - LA64/RISCV64 both allow splitting of 16-byte structs across 1 register and stack


For the splitting, is only the register saved on the stack within the prolog?
Should the second part of the struct which passed on caller's stack be copyed to on the prolog stack?
If don't copy the second part, how to process this part as the two parts are not stored on a continue stack space.

Yes, for arm32/arm64 we only need to store the register part. The stack segments are always at the beginning and will be contiguous with the register segments. I thought LA64/RV64 was similar here.
@tomeksowi is enabling code in #101288 to handle it more generally. LA64 can do the same.

Out of curiosity, can you show the ABI information JITDUMP for the case with the split segments that doesn't have the stack segment first? For win-arm64 I think the split args were deliberately designed this way to make varargs possible to handle.

For the old-style ABI, LA64 will copy the second part to the prolog stack where the splitting struct's size is the whole size and had allocated the whole stack space, that is, we can copy the second part directly.

I'm finding the test case, but I think/doubt the new-style can not process this case as we expected liking the old-style.

I had found this case.
Late I will add this case.

; V00 arg0 [V00 ] ( 1, 1 ) int -> [fp+0x54] do-not-enreg[] ; V01 arg1 [V01 ] ( 1, 1 ) struct (16) [fp+0x40] do-not-enreg[XS] addr-exposed ld-addr-op <Point> ; V02 arg2 [V02 ] ( 1, 1 ) struct (16) [fp+0x30] do-not-enreg[XS] addr-exposed ld-addr-op <Point> ; V03 arg3 [V03 ] ( 1, 1 ) struct (16) [fp+0x20] do-not-enreg[XS] addr-exposed ld-addr-op <Point> ; V04 arg4 [V04 ] ( 1, 1 ) struct (16) [fp+0x10] do-not-enreg[XS] addr-exposed ld-addr-op <Point> ;# V05 OutArgs [V05 ] ( 1, 1 ) struct ( 0) [sp+0x00] do-not-enreg[XS] addr-exposed "OutgoingArgSpace" ; ; Lcl frame size = 80 DEBUG: LOONGARCH64, frameType:1 G_M40687_IG01: ;; offset=0x0000 0xff7575a760 02FE8063 addi.d sp, sp, -96 0xff7575a764 29C00061 st.d ra, sp, 0 0xff7575a768 29C02076 st.d fp, sp, 8 0xff7575a76c 02C02076 addi.d fp, sp, 8 0xff7575a770 298152C4 st.w a0, fp, 84 0xff7575a774 29C102C5 st.d a1, fp, 64 0xff7575a778 29C122C6 st.d a2, fp, 72 0xff7575a77c 29C0C2C7 st.d a3, fp, 48 0xff7575a780 29C0E2C8 st.d a4, fp, 56 0xff7575a784 29C082C9 st.d a5, fp, 32 0xff7575a788 29C0A2CA st.d a6, fp, 40 0xff7575a78c 29C042CB st.d a7, fp, 16 0xff7575a790 28C1806C ld.d t0, sp, 96 // the old-style ABI will copy, but now the new stype not. 0xff7575a794 29C062CC st.d t0, fp, 24 // the old-style ABI will copy, but now the new stype not.

OK.
Maybe it's better to split to some PRs before 101288 if there are much modification.

I'm a bit apprehensive about merging a PR which breaks everything with split struct args. Maybe I'll handle split args in #101288 and then LA can enable the #ifdefs as well? I don't think there will be that much modification but I don't have a solution fully fleshed out yet.

Do you mean this PR? I don't think this PR will affect any behavior for either LA64/RISCV64. The only break right now came from #101224.

The original code computes the split stack parameters at similar offsets as this PR does for both LA64/RISCV64:

runtime/src/coreclr/jit/lclvars.cpp

Lines 6352 to 6362 in bc9fc5a

#elif defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)

if (varDsc->lvIsSplit)

{

assert((varDsc->lvType == TYP_STRUCT) && (varDsc->GetOtherArgReg() == REG_STK));

// This is a split struct. It will account for an extra (8 bytes) for the whole struct.

varDsc->SetStackOffset(varDsc->GetStackOffset() + TARGET_POINTER_SIZE);

argOffs += TARGET_POINTER_SIZE;

}

#else // TARGET*

(In fact, this was one of the primary reasons I thought LA64/RV64 already worked this way for split parameters -- but the stack offset set there is never used.)

It's possible I misunderstood what that code there is doing. Regardless, I don't think there is an issue with this PR since the offset this PR might be assigning to split parameters are getting overwritten regardless.

It's possible I misunderstood what that code there is doing.

I think I didn't express clearly to make you understand.

Regardless, I don't think there is an issue with this PR since the offset this PR might be assigning to split parameters are getting overwritten regardless.

Yes, what I feedback is not about this PR and I don't think LA64/RV64's splitting error is about this PR.
We just discussed here with you.
You can proceed this PR with your plan.

…ts-new-abi

jakobbotsch · 2024-05-03T09:39:43Z

Ping @kunalspathak -- can you please take a look?

kunalspathak

LGTM

…otnet#101340) Rewrite `lvaAssignVirtualFrameOffsetsToArgs` to make use of the ABI information that was already computed as part of ABI classification in the frontend.

JIT: Rewrite initial parameter frame layout in terms of new ABI info

aba8f69

Rewrite `lvaAssignVirtualFrameOffsetsToArgs` to make use of the ABI information that was already computed as part of ABI classification in the frontend.

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 20, 2024

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

jakobbotsch added 5 commits April 20, 2024 21:10

Fix build

f93287a

Fix validation for Swift, small clean ups

0500918

Remove stack alignment

89e00cd

Considering the stack alignment to be part of the passed stack size does not really make sense since the callee does not own the extra bytes added to ensure alignment.

Fix build

06173e4

Fix ifdef

b89dc82

jakobbotsch marked this pull request as ready for review April 22, 2024 09:27

jakobbotsch requested review from BruceForstall and kunalspathak April 22, 2024 09:28

Remove unnecessary change

451855f

jakobbotsch mentioned this pull request Apr 22, 2024

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

Merged

shushanhf reviewed Apr 23, 2024

View reviewed changes

shushanhf mentioned this pull request Apr 23, 2024

[LoongArch64] add new ABI classifier. #101224

Merged

jakobbotsch added 4 commits April 27, 2024 11:57

Merge branch 'main' of github.com:dotnet/runtime into arg-stack-offse…

73224bc

…ts-new-abi

Adjust some comments for RISCV64/LA64

7922f61

Missed a word

2ce06c0

Fix another comment

ec9fcfe

jakobbotsch closed this May 3, 2024

jakobbotsch reopened this May 3, 2024

build-analysis bot mentioned this pull request May 3, 2024

System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanScalarDestination_SpecialValues fails #101721

Closed

Merge branch 'main' into arg-stack-offsets-new-abi

ac09eb2

kunalspathak approved these changes May 7, 2024

View reviewed changes

jakobbotsch merged commit 562457f into dotnet:main May 8, 2024
105 of 107 checks passed

jakobbotsch deleted the arg-stack-offsets-new-abi branch May 8, 2024 08:22

EgorBo mentioned this pull request May 16, 2024

[Perf] Linux/arm64: 1 Improvement on 5/8/2024 8:51:10 AM dotnet/perf-autofiling-issues#34396

Closed

github-actions bot locked and limited conversation to collaborators Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Rewrite initial parameter frame layout in terms of new ABI info #101340

JIT: Rewrite initial parameter frame layout in terms of new ABI info #101340

jakobbotsch commented Apr 20, 2024 •

edited

Loading

dotnet-policy-service bot commented Apr 20, 2024

jakobbotsch commented Apr 22, 2024 •

edited

Loading

shushanhf Apr 23, 2024

jakobbotsch Apr 23, 2024 •

edited

Loading

shushanhf Apr 23, 2024

shushanhf Apr 23, 2024

shushanhf Apr 23, 2024

shushanhf Apr 24, 2024

tomeksowi Apr 24, 2024 •

edited

Loading

jakobbotsch Apr 24, 2024

jakobbotsch Apr 24, 2024

shushanhf Apr 25, 2024

jakobbotsch commented May 3, 2024

kunalspathak left a comment

	#elif defined(TARGET_LOONGARCH64) \|\| defined(TARGET_RISCV64)

	if (varDsc->lvIsSplit)
	{
	assert((varDsc->lvType == TYP_STRUCT) && (varDsc->GetOtherArgReg() == REG_STK));
	// This is a split struct. It will account for an extra (8 bytes) for the whole struct.
	varDsc->SetStackOffset(varDsc->GetStackOffset() + TARGET_POINTER_SIZE);
	argOffs += TARGET_POINTER_SIZE;
	}

	#else // TARGET*

JIT: Rewrite initial parameter frame layout in terms of new ABI info #101340

JIT: Rewrite initial parameter frame layout in terms of new ABI info #101340

Conversation

jakobbotsch commented Apr 20, 2024 • edited Loading

dotnet-policy-service bot commented Apr 20, 2024

jakobbotsch commented Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

jakobbotsch Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomeksowi Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented May 3, 2024

kunalspathak left a comment

Choose a reason for hiding this comment

jakobbotsch commented Apr 20, 2024 •

edited

Loading

jakobbotsch commented Apr 22, 2024 •

edited

Loading

jakobbotsch Apr 23, 2024 •

edited

Loading

tomeksowi Apr 24, 2024 •

edited

Loading