JIT: Use new ABI classifiers for `GenTreeCall` arguments #103537

jakobbotsch · 2024-06-16T10:28:34Z

Switch AddFinalArgsAndDetermineABIInfo to reuse the new-style ABI classifiers.

Does not remove the old ABI information representation in CallArg, but keeps both for now.

jakobbotsch · 2024-06-16T12:15:55Z

@dotnet/samsung can you please submit a PR to my branch that fixes the RISC-V failure?

This change is red in CI because it breaks the RISC-V targeted JIT that runs as part of CI because the build job runs crossgen2. Since this is not a supported (by Microsoft) configuration, I do not think the crossgen2 part should be running in our CI. Thoughts on this @jkotas?

jkotas · 2024-06-16T15:19:08Z

Since this is not a supported (by Microsoft) configuration, I do not think the crossgen2 part should be running in our CI. Thoughts on this @jkotas?

To prevent breaks in community supported targets, we have been open to run them in CI as long as it is cheap, stable and actively maintained. It seems to be working as expected here.

jakobbotsch · 2024-06-16T15:54:48Z

Since this is not a supported (by Microsoft) configuration, I do not think the crossgen2 part should be running in our CI. Thoughts on this @jkotas?

To prevent breaks in community supported targets, we have been open to run them in CI as long as it is cheap, stable and actively maintained. It seems to be working as expected here.

I am fine with having the build in CI. It is simple and straightforward and non-blocking to fix regular build failures.

Actually running the JIT as part of default CI is another question entirely if it means we can end up in a situation where we a blocked from making changes in our supported targets. It means I either 1) have to spend time debugging and fixing RISC-V, or 2) waiting and hoping for the maintainers to submit the necessary fixes for my own changes to make it in.

tomeksowi · 2024-06-16T16:09:46Z

@dotnet/samsung can you please submit a PR to my branch that fixes the RISC-V failure?

runtime/src/coreclr/jit/morph.cpp

Lines 2207 to 2214 in d37cba7

    
           assert(abiInfo.HasAnyStackSegment()); 
        
           // We only expect to see one stack segment in these cases. 
        
           assert(abiInfo.NumSegments == 1); 
        
           // This is a stack argument 
        
           m_hasStackArgs                   = true; 
        
           const ABIPassingSegment& segment = abiInfo.Segments[0]; 
        
           arg.AbiInfo.SetRegNum(0, REG_STK); 
        
           arg.AbiInfo.ByteOffset = segment.GetStackOffset();

I thought arguments passed on the stack were supposed to be split into stack slots. Anyway, seems a simple fix, I'll do it on Monday morning (CET) to classify as single segment.

jakobbotsch · 2024-06-16T16:20:55Z

@tomeksowi Thanks! It looks like I'll have to investigate throughput a bit anyway, so for now the RISC-V problem isn't the only problem with this change.

jakobbotsch · 2024-06-16T16:41:04Z

It seems to be working as expected here.

I think the expectation is that the breaks we may accidentally introduce are easily fixable (by anyone). This is not the case if we are running the JIT in CI.

Has your opinion on this changed since #84834 (comment)?

jakobbotsch · 2024-06-16T17:37:55Z

Also, I apologize if I am coming off as hostile to open-source here. I love working on the JIT and reviewing the contributions to the community maintained targets. I think the JIT is overall better as a result of them and moving in an awesome direction that is much more consistent and works better for everyone involved.

For me, it is not about this concrete instance (this change is not time sensitive), but more about the separation of responsibility, and what the expectations are of individual JIT contributors.

jkotas · 2024-06-16T21:38:57Z

Has your opinion on this changed since #84834 (comment)?

Let me expand on that:

If we want active community targets to developed upstream, we have to ensure that upstream is reasonably productive place for their development. The bar for "reasonably productive" rises with number of people working on the given target. The math for 1 person not being able to do work because of broken upstream vs. 10 people not being able to do work because of broken upstream is very different.

The alternatively would be to guide community targets towards doing most of the work in their own forks that are gated against breakages and integrate their changes upstream as code bombs every once in a while. I do not think that it would be the preferred way for everybody involved.

My primary concern is controlling the costs and reliability. It limits the validation to build-only targets in our current CI system. These build-only targets can include running cross-targeting AOT compilers. If we had a cheap (both in terms of hardware and labor) and reliable way to run tests in CI for actively maintained community targets, I would not be opposed to even running some tests in the CI.

have to spend time debugging and fixing RISC-V, or 2) waiting and hoping for the maintainers to submit the necessary fixes for my own changes to make it in.

Right. I do not think (2) is a problem for RISC-V port that is very actively maintained as demonstrated by the discussion in this PR. In case the maintainers of the port are not responsive, there is also (3) disable validation that is preventing progress.

…103538) Two changes: 1. Skip applying alignment when computing a call's arg stack size, which does not make sense/is not correct 2. Round up the incoming parameter stack space in terms of number of slots Without (1) we overestimate the stack size usage for some calls. Without (2) we underestimate the incoming stack size for some methods. Both of these just result in fewer tailcalls than possible, so cause no correctness issues. However, I hit some diffs in #103537 because of them.

jakobbotsch · 2024-06-18T06:46:25Z

Has your opinion on this changed since #84834 (comment)?

Let me expand on that:

If we want active community targets to developed upstream, we have to ensure that upstream is reasonably productive place for their development. The bar for "reasonably productive" rises with number of people working on the given target. The math for 1 person not being able to do work because of broken upstream vs. 10 people not being able to do work because of broken upstream is very different.

The alternatively would be to guide community targets towards doing most of the work in their own forks that are gated against breakages and integrate their changes upstream as code bombs every once in a while. I do not think that it would be the preferred way for everybody involved.

My primary concern is controlling the costs and reliability. It limits the validation to build-only targets in our current CI system. These build-only targets can include running cross-targeting AOT compilers. If we had a cheap (both in terms of hardware and labor) and reliable way to run tests in CI for actively maintained community targets, I would not be opposed to even running some tests in the CI.

have to spend time debugging and fixing RISC-V, or 2) waiting and hoping for the maintainers to submit the necessary fixes for my own changes to make it in.

Right. I do not think (2) is a problem for RISC-V port that is very actively maintained as demonstrated by the discussion in this PR. In case the maintainers of the port are not responsive, there is also (3) disable validation that is preventing progress.

I agree with everything you've said. I think that makes a lot of sense.

…#4) * Classify stack arguments as single segment * Bring back type fix-ups for structs passed according to hardware floating-point calling convention * Use more strict reg masking condition for fixed refs like on other platforms because after dotnet#97368 we don't need an exception

…ate fields

jakobbotsch · 2024-06-20T18:00:11Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs. Larger TP regressions than I would have hoped, but I did some TP work in #103761 and #103763 to make up for (some of) it. For the collections with a reasonable amount of MinOpts contexts I think it gets us to an acceptable level to take this unification.

We should also be able to get some of this back (but we won't get all of it) once we move other parts of the JIT to use the new ABI information directly so that we can get rid of the old one entirely.

AndyAyersMS · 2024-06-20T21:58:01Z

src/coreclr/jit/abi.cpp

@@ -206,7 +206,7 @@ ABIPassingInformation::ABIPassingInformation(Compiler* comp, unsigned numSegment

    if (numSegments > 1)
    {
-        Segments = new (comp, CMK_ABI) ABIPassingSegment[numSegments];
+        m_segments = new (comp, CMK_ABI) ABIPassingSegment[numSegments];


Is there any reason you can't always use the built in segment for segment[0] and allocate one less entry here?

m_segments and m_singleSegment is in a union currently. I didn't check whether it was overall worth it memory wise to separate those fields.. It would increase memory usage for the 1 segment case by a bit while decreasing memory usage in multiple segment cases by a bit more.

jakobbotsch · 2024-06-21T08:58:27Z

Memory impact of this change on win-x64 is around 0.6%: https://www.diffchecker.com/3LqCNszj/
That too should go down once we move everything off the old representation and remove it.

Switch `AddFinalArgsAndDetermineABIInfo` to reuse the new-style ABI classifiers. Does not remove the old ABI information representation in `CallArg`, but keeps both for now. Co-authored-by: Tomasz Sowiński <tomeksowi@gmail.com>

jakobbotsch added 8 commits June 15, 2024 09:19

JIT: Start using new-style ABI classifiers for call args

dc85759

Run jit-format

a95f745

Fix x86 special tailcall args

c1cdad4

Round up stack segments in old ABI info

49d9e6e

Fix multiple regs

cfe0f1d

Reorder HFA

11a3bc2

Quirk reg counting for arm32

03d33c0

Run jit-format

7861116

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

Clean up

693a980

dotnet-policy-service bot assigned jakobbotsch Jun 16, 2024

jakobbotsch mentioned this pull request Jun 16, 2024

JIT: Make stack size computation in fgCanFastTailCall more precise #103538

Merged

Run jit-format

00f5bbc

Compute ByteSize the old way

d37cba7

build-analysis bot mentioned this pull request Jun 16, 2024

NoWorkload-NoWebcil-ST-Wasm.Build.Tests.WorkItemExecution #103114

Closed

This was referenced Jun 17, 2024

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

Merged

[RISC-V] Classify stack arguments as single segment #103619

Merged

jakobbotsch mentioned this pull request Jun 19, 2024

JIT: Store one segment inline in ABIPassingInformation #103690

Merged

tomeksowi and others added 2 commits June 20, 2024 10:48

Merge branch 'main' of github.com:dotnet/runtime into call-args-new-abi

0ad6f6b

jakobbotsch added 4 commits June 20, 2024 11:23

Fix build

221d464

Fix RISC-V build

fe7d665

Copy new ABI info in CallArgs::InternalCopyFrom

b8595f0

Fix HasAnyFloatingRegisterSegment for new representation, rename priv…

46e461b

…ate fields

jakobbotsch mentioned this pull request Jun 20, 2024

JIT: Specialize fgFindJumpTargets #103761

Merged

jakobbotsch marked this pull request as ready for review June 20, 2024 17:58

jakobbotsch requested a review from AndyAyersMS June 20, 2024 18:00

AndyAyersMS approved these changes Jun 20, 2024

View reviewed changes

am11 mentioned this pull request Jun 21, 2024

Default DynamicCodeSupport to true for CoreCLR #103594

Merged

jakobbotsch merged commit 42ade70 into dotnet:main Jun 21, 2024
107 checks passed

jakobbotsch deleted the call-args-new-abi branch June 21, 2024 08:59

github-actions bot locked and limited conversation to collaborators Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Use new ABI classifiers for `GenTreeCall` arguments #103537

JIT: Use new ABI classifiers for `GenTreeCall` arguments #103537

jakobbotsch commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

jkotas commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

tomeksowi commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024 •

edited

Loading

jakobbotsch commented Jun 16, 2024

jkotas commented Jun 16, 2024 •

edited

Loading

jakobbotsch commented Jun 18, 2024

jakobbotsch commented Jun 20, 2024 •

edited

Loading

AndyAyersMS Jun 20, 2024

jakobbotsch Jun 21, 2024

jakobbotsch commented Jun 21, 2024

JIT: Use new ABI classifiers for GenTreeCall arguments #103537

JIT: Use new ABI classifiers for GenTreeCall arguments #103537

Conversation

jakobbotsch commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

jkotas commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

tomeksowi commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024

jakobbotsch commented Jun 16, 2024 • edited Loading

jakobbotsch commented Jun 16, 2024

jkotas commented Jun 16, 2024 • edited Loading

jakobbotsch commented Jun 18, 2024

jakobbotsch commented Jun 20, 2024 • edited Loading

AndyAyersMS Jun 20, 2024

Choose a reason for hiding this comment

jakobbotsch Jun 21, 2024

Choose a reason for hiding this comment

jakobbotsch commented Jun 21, 2024

JIT: Use new ABI classifiers for `GenTreeCall` arguments #103537

JIT: Use new ABI classifiers for `GenTreeCall` arguments #103537

jakobbotsch commented Jun 16, 2024 •

edited

Loading

jkotas commented Jun 16, 2024 •

edited

Loading

jakobbotsch commented Jun 20, 2024 •

edited

Loading