JIT: Refactor call argument representation #67238

jakobbotsch · 2022-03-28T15:43:18Z

This refactors the JIT's representation of call arguments. It replaces
GenTreeCall::Use and fgArgTabEntry with a single class CallArg.
CallArg always contains space for ABI information and contains two
intrusive linked list nodes: one for all args (similar to current
gtCallArgs) where all standard arguments are always in argument order,
and one for late args (similar to current gtCallLateArgs) that may be
reordered. The late args list may also not contain all arguments.

fgArgInfo is also replaced by a new class CallArgs that is stored
inline in GenTreeCall. It encapsulates all handling of arguments
(insertion/removal). The change also begins treating the 'this' argument
as a normal argument with CallArgs providing convenient access to it
when necessary.

The main benefit of this change is to avoid keeping track of the side
table fgArgInfo and having to scan through this side table repeatedly
when we need to query argument information. In addition it gives more
convenient ways to access well known arguments like 'this', the ret
buffer, VSD cell, etc.

Follow-up changes I want to make after this:

Remove quirks. In trying to keep this zero-diff several quirks were
added. In particular we currently miss a significant optimization
opportunity by not retyping 'this' arguments as unmanaged pointers
when they are on stack.
Remove GT_PUTARG_TYPE nodes. These are used only to transfer
information from import to morph. Now that we always allocate argument
information we can just store this information directly.
Remove GTF_LATE_ARG
Remove CallArg::ArgNum, not necessary since args are in order now
Remove DEBUG_ARG_SLOTS which adds some complication and has served its purpose

Since this is a large change I will wait until at least #62843 is merged before marking it as ready.
Also, I need to review this a couple of times and see testing.

Some important differences for JIT devs:

CallArg::GetNode() is equivalent to fgArgTabEntry::GetNode(). That means it is the late node if it is present. In particular it means that the following loops are not completely equivalent:

for (GenTreeCall::Use& use : call->gtCallArgs)
{
  GenTree* node = use.GetNode();
  // Do something with early node
}

for (CallArg& arg : call->gtArgs.Args())
{
  GenTree* node = arg.GetNode();
  // Node is actual argument node, not the setup node
}

This is intentional since I think it is a pitfall if GetNode() returns the setup node instead of the actual arg node. Also note that it is quite rare that we want access to the actual setup node after morph and they are equivalent before. The equivalent to the former loop is to use GetEarlyNode().

This refactors the JIT's representation of call arguments. It replaces `GenTreeCall::Use` and `fgArgTabEntry` with a single class `CallArg`. `CallArg` always contains space for ABI information and contains two intrusive linked list nodes: one for all args (similar to current `gtCallArgs`) where all standard arguments are always in argument order, and one for late args (similar to current `gtCallLateArgs`) that may be reordered. The late args list may also not contain all arguments. `fgArgInfo` is also replaced by a new class `CallArgs` that is stored inline in `GenTreeCall`. It encapsulates all handling of arguments (insertion/removal). The change also begins treating the 'this' argument as a normal argument with `CallArgs` providing convenient access to it when necessary. The main benefit of this change is to avoid keeping track of the side table `fgArgInfo` and having to scan through this side table repeatedly when we need to query argument information. In addition it gives more convenient ways to access well known arguments like 'this', the ret buffer, VSD cell, etc. Follow-up changes I want to make after this: * Remove quirks. In trying to keep this zero-diff several quirks were added. In particular we currently miss a significant optimization opportunity by not retyping 'this' arguments as unmanaged pointers when they are on stack. * Remove GT_PUTARG_TYPE nodes. These are used only to transfer information from import to morph. Now that we always allocate argument information we can just store this information directly.

ghost · 2022-03-28T15:43:26Z

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

Issue Details

This refactors the JIT's representation of call arguments. It replaces
GenTreeCall::Use and fgArgTabEntry with a single class CallArg.
CallArg always contains space for ABI information and contains two
intrusive linked list nodes: one for all args (similar to current
gtCallArgs) where all standard arguments are always in argument order,
and one for late args (similar to current gtCallLateArgs) that may be
reordered. The late args list may also not contain all arguments.

fgArgInfo is also replaced by a new class CallArgs that is stored
inline in GenTreeCall. It encapsulates all handling of arguments
(insertion/removal). The change also begins treating the 'this' argument
as a normal argument with CallArgs providing convenient access to it
when necessary.

The main benefit of this change is to avoid keeping track of the side
table fgArgInfo and having to scan through this side table repeatedly
when we need to query argument information. In addition it gives more
convenient ways to access well known arguments like 'this', the ret
buffer, VSD cell, etc.

Follow-up changes I want to make after this:

Remove quirks. In trying to keep this zero-diff several quirks were
added. In particular we currently miss a significant optimization
opportunity by not retyping 'this' arguments as unmanaged pointers
when they are on stack.
Remove GT_PUTARG_TYPE nodes. These are used only to transfer
information from import to morph. Now that we always allocate argument
information we can just store this information directly.

Since this is a large change I will wait until at least #62843 is merged before marking it as ready.
Also, I need to review this a couple of times and see testing.

Author:	jakobbotsch
Assignees:	jakobbotsch
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2022-03-28T15:58:37Z

src/coreclr/jit/flowgraph.cpp

@@ -1067,7 +1062,8 @@ GenTree* Compiler::fgOptimizeDelegateConstructor(GenTreeCall*            call,
    }
    else if (oper == GT_CALL && targetMethod->AsCall()->gtCallMethHnd == eeFindHelper(CORINFO_HELP_VIRTUAL_FUNC_PTR))
    {
-        GenTree* handleNode = targetMethod->AsCall()->gtCallArgs->GetNext()->GetNext()->GetNode();


Glad to see such ->GetNext()->GetNext()->GetNode() are removed 😄

Indeed, although I will have to review these carefully a few times since 'this' is now a regular arg, so some of these might have shifted.

jakobbotsch · 2022-03-28T17:01:37Z

FYI @dotnet/jit-contrib

src/coreclr/jit/importer.cpp

jakobbotsch · 2022-04-10T21:38:01Z

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr crossgen2 outerloop, Fuzzlyn, jit-cfg

azure-pipelines · 2022-04-10T21:38:42Z

Azure Pipelines successfully started running 5 pipeline(s).

jakobbotsch · 2022-04-11T12:33:00Z

The CI pipelines ran in https://github.com/dotnet/runtime/pull/67238/checks?check_run_id=5963207833 but I have committed some minor documentation changes since then. All the failures look preexisting, the majority being #67821. I believe this is ready -- zero diffs and I have reviewed it a couple of times myself.

cc @dotnet/jit-contrib, anyone wants to volunteer to review this?

Also cc @shushanhf -- there are a few changes in LoongArch64 for the new call arg representation that you may want to double check/test.

shushanhf · 2022-04-11T13:10:20Z

Also cc @shushanhf -- there are a few changes in LoongArch64 for the new call arg representation that you may want to double check/test.

Thanks, I will check it for LA.

jakobbotsch · 2022-04-11T13:15:27Z

PIN results on libraries.pmi for x64:
Before: 327070083563
After: 325054392352 (-0.6%)

I suspect most of this is due to avoiding the repeated linear searches for arg table entries. Will post memory stats when I'm back tomorrow.

jakobbotsch · 2022-04-12T13:19:15Z

Diff of memory stats over libraries.pmi (left: before, right: after): https://www.diffchecker.com/kFqhlvta

Before: 34513223680 bytes
After: 34400698368 bytes (-0.3%)

AndyAyersMS · 2022-04-13T02:39:13Z

I looked most everything over just to familiarize myself with the changes (except morph). Will go back through in more detail tomorrow and write up some specific feedback.

Overall I agree this looks like a very nice cleanup.

Seems like we use the "enumerate early args then late args" pattern quite often, I wonder if some sort of combined iterator makes sense.

Also wonder if we can come up with some kind of safeguards to make it harder to look at the wrong enumerations. Or maybe find better descriptive names?

jakobbotsch · 2022-04-13T08:48:58Z

Seems like we use the "enumerate early args then late args" pattern quite often, I wonder if some sort of combined iterator makes sense.

I think for the most part these cases are "visit all operands of GenTreeCall nodes" and we should probably look into making them use either GenTree::UseEdges, GenTree::Operands or GenTreeVisitor. Also if the ordering is not important we can just use Args() and access both GetEarlyNode() and GetLateNode() in the loop body. However, I think this is a cleanup for another PR.

Also wonder if we can come up with some kind of safeguards to make it harder to look at the wrong enumerations. Or maybe find better descriptive names?

I agree, it would be nice to find a better name for LateArgs that makes it clear that this is a subset of Args and that it may not be in arg order. ArgsNeedingLatePlacement could be one, although a bit verbose...
I do think with this PR the situation is significantly better than before. Iterating Args() and using GetNode() is in almost all scenarios the thing you want (it will never give you a placeholder/setup node). Contrast that with before, where iterating gtCallArgs and using GetNode() would give you setup nodes/placeholder after morph.

Note that many places in this PR are using GetEarlyNode() before morph which is always the same node as GetNode(). New code should probably just use GetNode() here, but when I mass-converted the uses I attempted to make sure the semantics were the same as previously. Perhaps something I might look at simplifying in a future PR.

AndyAyersMS

Have looked through everything but morph.cpp.

src/coreclr/jit/flowgraph.cpp

src/coreclr/jit/gentree.cpp

AndyAyersMS · 2022-04-13T16:46:32Z

src/coreclr/jit/gentree.cpp

+GenTreeCall* Compiler::gtNewCallNode(gtCallTypes           callType,
+                                     CORINFO_METHOD_HANDLE callHnd,
+                                     var_types             type,
+                                     const DebugInfo&      di)


We're no longer setting node flags based on arg flags.

Presumably this now happens up in the callers? Or does it not matter much since GTF_CALL usually implies all the other side effects?

Yes, this should happen in callers now.
In general the arg list here was only used interestingly for JIT helpers, so I have folded them in as optional arguments in gtNewHelperCallNode that takes care of setting the flags.

For user calls, impImportCall will use impPopCallArgs and set the flags from the arguments after this. With that said, I have reviewed the sites that use impPopCallArgs and it looks like I have missed the flags in a couple of places... will fold it into impPopCallArgs.

I have also considered making CallArgs handle it when inserting arguments. It should be easy to get a back-pointer to the parent GenTreeCall given that it is stored inline. I think it would be nice not to be able to forget, but also it is conceptually wrong to do this after rationalization, so I am not totally sure if it makes sense to do always.

FWIW, initially I wanted to replace the previous argument with std::initializer_list, but we cannot use STL :-(

AndyAyersMS · 2022-04-13T18:06:33Z

src/coreclr/jit/importer.cpp

+                                // TODO-CallArgs-REVIEW: Might discard commas otherwise?
+                                assert(thisObj == thisArg->GetEarlyNode());
+                                thisArg->SetEarlyNode(localCopyThis);


Yes, please open an issue.

src/coreclr/jit/morph.cpp

AndyAyersMS · 2022-04-14T15:58:55Z

src/coreclr/jit/morph.cpp

@@ -2291,36 +1910,25 @@ GenTree* Compiler::fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE
    return new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, subTree->TypeGet(), lclNum);
 }

-//------------------------------------------------------------------------
-// fgInitArgInfo: Construct the fgArgInfo for the call with the fgArgEntry for each arg
+// AddFinalArgsAndDetermineABIInfo:


Add back the

//---------

line and document the args

Are we dead set on the full template everywhere?
In this case the documentation would be something like "comp - the compiler" and "call - the call", which is not super useful and just adds noise. I have in many cases omitted documenting args when it would not clarify anything.

It is our convention, yes.

I usually add them even if I think the args are self-descriptive, to at least distinguish those cases from the ones where the args might seem to be self-descriptive but aren't.

Ok, let me include these as part of one of the follow-up changes to avoid rerunning CI for it.

src/coreclr/jit/morph.cpp

The early arg could be a local that was optimized to a constant later

Remove one of the quirks added in dotnet#67238.

ghost assigned jakobbotsch Mar 28, 2022

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

jakobbotsch added 5 commits March 28, 2022 17:46

Run jit-format

8b82719

Value number arg fix

e6d9776

Undo clang-format change

3b83e3c

Reorder

5fd50ed

Use helper

80e4136

EgorBo reviewed Mar 28, 2022

View reviewed changes

Fix build

eb938a5

jakobbotsch added 15 commits March 28, 2022 19:05

Increase space before nodes to fit call arg names

53e9e3c

Rename memory kind, fix Unix x86 build, remove occurences of fgArgInfo

922f9d8

Remove some more mentions of outdated names

c360a4f

Fix assert on macOS arm64

44e22cc

Fixes for NativeAOT

421f21f

Another fix

ece3b59

Run jit-format

51c7454

Small cleanup

820650c

Another small cleanup

7235c64

More cleanup

a337083

Cleanup

12819b0

Remove some commented code

053cf9f

Cleanup

b5ab444

Remove unnecessary ctor call

6e228d3

Remove unused declaration

42bd8c1

jakobbotsch commented Mar 28, 2022

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

Fix a comment

dbf30a8

This comment was marked as spam.

Sign in to view

jakobbotsch added 2 commits April 11, 2022 14:28

Small documentation clean up

9b4003d

Reformat

b190869

jakobbotsch marked this pull request as ready for review April 11, 2022 12:34

jakobbotsch requested a review from AndyAyersMS April 12, 2022 16:25

AndyAyersMS reviewed Apr 13, 2022

View reviewed changes

Address feedback, propagate arg flags properly

ff75346

jakobbotsch mentioned this pull request Apr 13, 2022

JIT: impDevirtualizeCall might discard commas #67982

Open

Minor change and rerun failing CI

c53434c

AndyAyersMS reviewed Apr 14, 2022

View reviewed changes

Address feedback

8e38f26

This was referenced Apr 14, 2022

System.Runtime.Serialization.Xml.Tests failed #59926

Closed

baseservices\\exceptions\\stackoverflow\\stackoverflowtester\\stackoverflowtester.cmd #65201

Closed

Revert check for this needing temp on early expanded calls

722c5d7

The early arg could be a local that was optimized to a constant later

AndyAyersMS approved these changes Apr 15, 2022

View reviewed changes

jakobbotsch merged commit dbf5c58 into dotnet:main Apr 15, 2022

jakobbotsch deleted the refactor-call-args branch April 15, 2022 08:18

jakobbotsch added a commit to jakobbotsch/runtime that referenced this pull request Apr 15, 2022

Retype pointers to 'this' on stack as unmanaged pointers

e47c27a

Remove one of the quirks added in dotnet#67238.

This was referenced Apr 15, 2022

Retype pointers to 'this' on stack as unmanaged pointers #68074

Merged

Fix JITDUMP alignment of promoted fields #68076

Merged

jakobbotsch mentioned this pull request Apr 27, 2022

Make sure that GenTree pointer is stored only in its parent. #13293

Closed

ghost locked as resolved and limited conversation to collaborators May 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Refactor call argument representation #67238

JIT: Refactor call argument representation #67238

jakobbotsch commented Mar 28, 2022 •

edited

Loading

ghost commented Mar 28, 2022

EgorBo Mar 28, 2022

jakobbotsch Mar 28, 2022

jakobbotsch commented Mar 28, 2022

This comment was marked as spam.

jakobbotsch commented Apr 10, 2022

azure-pipelines bot commented Apr 10, 2022

jakobbotsch commented Apr 11, 2022

shushanhf commented Apr 11, 2022

jakobbotsch commented Apr 11, 2022

jakobbotsch commented Apr 12, 2022

AndyAyersMS commented Apr 13, 2022

jakobbotsch commented Apr 13, 2022

AndyAyersMS left a comment

AndyAyersMS Apr 13, 2022

jakobbotsch Apr 13, 2022

jakobbotsch Apr 13, 2022

AndyAyersMS Apr 13, 2022

AndyAyersMS Apr 14, 2022

jakobbotsch Apr 14, 2022

AndyAyersMS Apr 15, 2022

jakobbotsch Apr 15, 2022

JIT: Refactor call argument representation #67238

JIT: Refactor call argument representation #67238

Conversation

jakobbotsch commented Mar 28, 2022 • edited Loading

Some important differences for JIT devs:

ghost commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Mar 28, 2022

This comment was marked as spam.

jakobbotsch commented Apr 10, 2022

azure-pipelines bot commented Apr 10, 2022

jakobbotsch commented Apr 11, 2022

shushanhf commented Apr 11, 2022

jakobbotsch commented Apr 11, 2022

jakobbotsch commented Apr 12, 2022

AndyAyersMS commented Apr 13, 2022

jakobbotsch commented Apr 13, 2022

AndyAyersMS left a 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

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

jakobbotsch commented Mar 28, 2022 •

edited

Loading