Keep precise argument sizes between import and morph. #43130

sandreenko · 2020-10-07T07:53:10Z

On arm64 OSX we need to know precise argument sizes. I have added support for that between morph and lowering in #42503, this PR supports in between importation and morph.

When do we need it:

IL: load byte; call1(int) -> IR: CALL1(IND byte);
IL load int; call2(byte) -> IR: CALL2(IND int);

without the change, morph was seeing the incorrect type of the nodes but for other platforms it was not important (because all arguments were passed in TARGET_POINTER_SIZE slots).

I have stopped on the solution that adds a special GT_OPER to keep this information between these phases and a debug code in morph to check that it has survived phases between them (mostly inlining). Thanks @CarolEidt for the idea.
I want to enable this code for all platforms because:

do not want to introduce platform dependency in HIR;
it will make arm64 OSX work cheaper because we will catch bugs in this component on other platforms until we have arm64 OSX in ci;
the overhead is small, it should not affect TP;

I was also considering other solutions, the main 2:

add a new artificial structure to keep this information, map/dictionary, but

we can't increase call node size;
it will be an error-prone component from our previous experience with fgArgInfo;
it will have non-const access time or memory overhead, also non-trivial copying;

change impImplicitIorI4Cast to insert cast not only between i4 and i8 but between all sizes, but

we don't need it if it is not under a call;
it will require changes during inlining to ignore such nodes;
it will be easy for such nodes to slip through morph phase;

~~No asm diffs (SPMI x64/x86).~~ Update: ci shows related failures during the check phase, I will fix them soon, but I think it should not block review.

sandreenko · 2020-10-12T07:22:44Z

src/coreclr/src/jit/importer.cpp


            if (corType != CORINFO_TYPE_CLASS && corType != CORINFO_TYPE_BYREF && corType != CORINFO_TYPE_PTR &&
-                corType != CORINFO_TYPE_VAR && (argRealClass = info.compCompHnd->getArgClass(sig, argLst)) != nullptr)


I wonder why we did not see a warning about an assignment inside if.

sandreenko · 2020-10-12T07:25:37Z

src/coreclr/src/jit/morph.cpp

+    // load byte; call(int) -> CALL(PUTARG_TYPE byte(IND byte));
+    // load int; call(byte) -> CALL(PUTARG_TYPE int (IND int));
+    // etc.
+    if ((call->callSig != nullptr) && (Target::g_tgtArgOrder == Target::ARG_ORDER_R2L))


callSig == nullptr means mostly a helper call, all helper calls that I have checked are working with i4 and i8 types but I expect I will have to add more PUTARG_TYPE there once I run more tests on arm64 OSX and find bugs with missed type information.

sandreenko · 2020-10-12T07:26:08Z

PTAL @CarolEidt @AndyAyersMS @dotnet/jit-contrib

AndyAyersMS · 2020-10-12T18:20:13Z

I'm puzzled why we need to do things this way -- doesn't the callee sig tell us what size the callee expects?

sandreenko · 2020-10-12T18:26:05Z

I'm puzzled why we need to do things this way -- doesn't the callee sig tell us what size the callee expects?

We get this information from callee sig, but it is available only during importation and then we lose it. In debug we save it in

#ifdef DEBUG
    // Used to register callsites with the EE
    CORINFO_SIG_INFO* callSig;
#endif

if we decide to keep it in Release it will increase the size of all large nodes.

AndyAyersMS · 2020-10-12T18:39:33Z

Seems like we could deal with that easily enough, we have various extensions already hanging off of calls. For instance we could refactor TailCallSiteInfo which already holds onto a CORINFO_SIG_INFO and use it more broadly for cases where we need the sig later in compilation.

Is there other benefit to having these extra IR nodes beyond being able to handle this ABI quirk?

sandreenko · 2020-10-12T18:58:23Z

Is there other benefit to having these extra IR nodes beyond being able to handle this ABI quirk?

These extra nodes are only for the ABI quirk, I do not see benefits to other places from it. I was thinking to postpone casts creations but it was a long shot (if we pass 1 byte as 1 byte we don't need CAST int<-byte<-int).
I was using the signature in my prototype (from the debug field) to get the correct handle and there were some issues:

no way to handle calls without a signature if we get such;
not easy to match the signature to the actual argument nodes in morph. I am trying to do it for checking now, but with additional arguments from both sides of the list it is not trivial;
need to do extra calls to strip(info.compCompHnd->getArgType(sig, sigArgs, &classHnd)) (minor);

CarolEidt

Some initial comments and suggestions

src/coreclr/src/jit/flowgraph.cpp

CarolEidt · 2020-10-12T20:27:08Z

src/coreclr/src/jit/gentree.h

 {
    GenTree* effectiveVal = this;
    for (;;)
    {
+        assert(!effectiveVal->OperIs(GT_PUTARG_TYPE));


I'm curious why you added this here - that is, I can see why we wouldn't expect to encounter these under a COMMA, but I'm not sure why this special assert is needed.

We have several similar functions like gtEffectiveVal, gtRetExprVal and gtSkipPutArgType. When we call each of them we often don't expect nodes that can be skipped by two others to be on top.
For example, when we call gtEffectiveVal we don't expect something like GT_RET_EXPR(GT_COMMA) or GT_PUTARG_TYPE(GT_COMMA), similar for gtRetExprVal it should not see GT_PUTARG_TYPE(GT_PUTARG_TYPE).
If we do it means the caller did not check for these nodes and we don't skip what we hoped to skip, it doesn't cause correctness issues in my understanding but introduces asm regressions when we can't parse something like we did before the change.

src/coreclr/src/jit/importer.cpp

src/coreclr/src/jit/morph.cpp

CarolEidt · 2020-10-12T20:36:40Z

src/coreclr/src/jit/morph.cpp

+        // After tail call transformations we could have more nodes in the list + special arguments +
+        // setup trees. I need to find a clean way to match this list with the original signature if
+        // is possible. If not we will be left without a check that PUTARG_TYPE survive as they should.
+        if (nodeArgsCount == sigArgsCount)


Should this be !=? Otherwise, the loop below will never execute, right?

It is a temporary condition, the idea here is to match the signature with the call arguments as we have them in morph and check that we still have the correct types. However, it is tricky to do because we could have some nodes added to gtCallArgs between importation and call to this function (also this function can be called several times).
So we need to skip all additional nodes that were added and I have problems with identifying them. I am planning to fix it once I pass all the other tests.

src/coreclr/src/jit/rationalize.cpp

sandreenko · 2020-11-19T10:27:30Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2020-11-19T10:27:52Z

Azure Pipelines successfully started running 1 pipeline(s).

sandreenko · 2020-11-20T00:09:53Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2020-11-20T00:10:13Z

Azure Pipelines successfully started running 1 pipeline(s).

sandreenko

The PR was updated and is passing the tests now.
Thanks to @AndyAyersMS for helping me offline with the previous test failures.

The changes include #44790 that I hope to merge first.

This is the last big JIT change for arm64 apple, with it we will have precise arguments sizes from importer to codegen. The next changes will be on VM side for interop stubs.

sandreenko · 2020-11-20T00:50:20Z

src/coreclr/src/jit/importer.cpp

 {
    InlArgInfo* inlCurArgInfo = &pInlineInfo->inlArgInfo[argNum];

+    inlCurArgInfo->argNode = curArgVal; // Save the original tree, with PUT_ARG and RET_EXPR.


The main change is that we save PUTARG_TYPE and RET_EXPR in inlCurArgInfo->argNode.

sandreenko · 2020-11-20T00:52:03Z

src/tests/JIT/Regression/JitBlue/Runtime_43130/Runtime_43130.il

+    .maxstack  8
+    IL_0000:  nop
+    IL_0001:  ldarg.0
+    IL_0002:  call       void Runtime_43130::CallAMethod1(int16)


int32 is passed as int16 to Inline1 where it is passed to CallAMethod1, before the last changes the saved node was LCL_VAR int and we added another PUTARG_TYPE on top of an existing one, now it is fixed.

sandreenko · 2020-11-20T01:11:34Z

src/coreclr/src/jit/flowgraph.cpp

@@ -23820,7 +23848,8 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)

                GenTree* argSingleUseNode = argInfo.argBashTmpNode;

-                if ((argSingleUseNode != nullptr) && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) && argIsSingleDef)
+                if ((argSingleUseNode != nullptr) && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) && argIsSingleDef &&
+                    !argHasPutArg)


This new condition allows us to always the argument as a temp with the correct type, we need it because for a correct direct substitution we need to know if the parent is a call or not but we don't have this information.
We create the node when we see an IL opcode like ldlocal and don't have parent information, later, when the node is used, we can't distinguish it from a regular LCL_VAR.
The change causes a small regression on all platforms:

windows x64 crossgen: 5134 (0.02% of base) windows x64 pmi: 14911 (0.03% of base) windows arm64 crossgen: 5688 (0.01% of base) linux x64 crossgen: 9036 (0.02% of base)

, etc. all lower than 0.04%.

What happens there is:

during importation we create a LCL_VAR tempForOurArg, save a pointer to it;

in fgInsertInlineeBlocks we iterate over all such pointers and do in-place replacement if there was only one use of the lclVar, but we don't know the user, so we can't say if it is a call that need a PUTARG or not.

I have not found a simple way to avoid it or compensate for it.

It would be good to have a comment in the code here explaining why argHasPutArg disqualifies this arg from direct substitution.

AndyAyersMS

Left you some questions about this change.

AndyAyersMS · 2020-11-20T01:36:18Z

src/coreclr/src/jit/flowgraph.cpp

@@ -5179,7 +5179,7 @@ void Compiler::fgObserveInlineConstants(OPCODE opcode, const FgStack& stack, boo
                    // Check for the double whammy of an incoming constant argument
                    // feeding a constant test.
                    unsigned varNum = FgStack::SlotTypeToArgNum(slot0);
-                    if (impInlineInfo->inlArgInfo[varNum].argNode->OperIsConst())
+                    if (impInlineInfo->inlArgInfo[varNum].argIsInvariant)


Do you need this change for correctness? If not perhaps you should PR it separately.

Ditto for the other two below.

I do not need this for correctness. I am planning to merge them in #44790 (comment), if we decide not to merge it I will need to change this code to use argNode->gtSkipPutArgType()->gtRetExprVal().

I just approved #44790.

AndyAyersMS · 2020-11-20T01:40:58Z

src/coreclr/src/jit/flowgraph.cpp

@@ -23820,7 +23848,8 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)

                GenTree* argSingleUseNode = argInfo.argBashTmpNode;

-                if ((argSingleUseNode != nullptr) && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) && argIsSingleDef)
+                if ((argSingleUseNode != nullptr) && !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) && argIsSingleDef &&
+                    !argHasPutArg)


It would be good to have a comment in the code here explaining why argHasPutArg disqualifies this arg from direct substitution.

src/coreclr/src/jit/compiler.h

src/coreclr/src/jit/flowgraph.cpp

AndyAyersMS · 2020-11-20T01:52:02Z

src/coreclr/src/jit/flowgraph.cpp

+            GenTree*          argNode        = inlArgInfo[argNum].argNode;
+            const bool        argHasPutArg   = argNode->OperIs(GT_PUTARG_TYPE);
+
+            unsigned __int64 bbFlags = 0;


See note above; I don't believe you can set these to zero like this and pass jit stress.

src/coreclr/src/jit/morph.cpp

src/coreclr/src/jit/flowgraph.cpp

sandreenko · 2020-11-20T10:16:16Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2020-11-20T10:16:35Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS

I think this is good to merge once you've updated for #44790 and perhaps added a comment indicating why PUTARG_TYPE can't be directly substituted.

Create `GT_PUTARG_TYPE` when signature type does not match node type. Check in morph that this information has survived inlining and other phases between.

sandreenko · 2020-11-25T00:44:17Z

I have checked that the diffs are still as expected before the merge, PR and jitstress are clear,
the only ci failure is due to #45168.

sandreenko · 2020-11-25T00:44:54Z

Thanks @AndyAyersMS and @CarolEidt for your help with this change.

sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 7, 2020

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch from 9d5efd5 to c986f65 Compare October 8, 2020 05:55

This was referenced Oct 8, 2020

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch 4 times, most recently from 0a0669b to 115e44c Compare October 12, 2020 07:02

sandreenko marked this pull request as ready for review October 12, 2020 07:03

sandreenko changed the title ~~Add GenTreePutArgSmall.~~ Keep precise argument sizes between import and morph. Oct 12, 2020

sandreenko commented Oct 12, 2020

View reviewed changes

CarolEidt reviewed Oct 12, 2020

View reviewed changes

This was referenced Oct 13, 2020

Incorrect IL is accepted by the Jit and leading to incorrect execution. #43342

Closed

Add an implicit argument coercion check. #43386

Merged

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch from 5e87bfb to 6b0b884 Compare November 6, 2020 19:43

sandreenko mentioned this pull request Nov 17, 2020

Use argIsInvariant instead of argNode->OperIsConst() for inlining observations. #44790

Merged

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch 2 times, most recently from 0fcee3c to ada8c7f Compare November 19, 2020 09:21

This was referenced Nov 19, 2020

Testing out the new system jaredpar/runfo#71

Open

Many Android JIT tests failing in CI #44306

Closed

sandreenko commented Nov 20, 2020

View reviewed changes

AndyAyersMS reviewed Nov 20, 2020

View reviewed changes

sandreenko commented Nov 20, 2020

View reviewed changes

src/coreclr/src/jit/flowgraph.cpp Show resolved Hide resolved

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch from 5b28654 to 05d3c2e Compare November 20, 2020 10:16

AndyAyersMS approved these changes Nov 23, 2020

View reviewed changes

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch 2 times, most recently from 64db1b2 to 133ef0d Compare November 24, 2020 09:48

runfoapp bot mentioned this pull request Nov 24, 2020

ClientAndServer_OneOrBothUseDefault_Ok - fails with NullReferenceException #2353

Closed

Keep precise argument sizes between import and morph.

8c7ecdb

Create `GT_PUTARG_TYPE` when signature type does not match node type. Check in morph that this information has survived inlining and other phases between.

sandreenko force-pushed the Arm64Apple-Jit-Part4 branch from 133ef0d to 8c7ecdb Compare November 24, 2020 20:51

sandreenko merged commit 3e65d68 into dotnet:master Nov 25, 2020

runfoapp bot mentioned this pull request Nov 25, 2020

Mono System.Text.Json.Tests on Windows timing out #42677

Open

This was referenced Dec 1, 2020

[Perf] Changes at 11/25/2020 1:25:27 AM DrewScoggins/performance-2#3543

Open

[Perf] Changes at 11/25/2020 1:25:27 AM DrewScoggins/performance-2#3551

Open

sandreenko mentioned this pull request Dec 2, 2020

osx-arm64 ABI changes. #41130

Closed

3 tasks

sandreenko mentioned this pull request Dec 11, 2020

[Perf -7%] System.Text.Perf_Utf8Encoding.GetBytes #45554

Closed

ghost locked as resolved and limited conversation to collaborators Dec 25, 2020

sandreenko deleted the Arm64Apple-Jit-Part4 branch December 29, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep precise argument sizes between import and morph. #43130

Keep precise argument sizes between import and morph. #43130

sandreenko commented Oct 7, 2020 •

edited

Loading

sandreenko Oct 12, 2020

sandreenko Oct 12, 2020

sandreenko commented Oct 12, 2020

AndyAyersMS commented Oct 12, 2020

sandreenko commented Oct 12, 2020

AndyAyersMS commented Oct 12, 2020

sandreenko commented Oct 12, 2020

CarolEidt left a comment

CarolEidt Oct 12, 2020

sandreenko Oct 13, 2020

CarolEidt Oct 12, 2020

sandreenko Oct 13, 2020

sandreenko commented Nov 19, 2020

azure-pipelines bot commented Nov 19, 2020

sandreenko commented Nov 20, 2020

azure-pipelines bot commented Nov 20, 2020

sandreenko left a comment

sandreenko Nov 20, 2020

sandreenko Nov 20, 2020

sandreenko Nov 20, 2020

AndyAyersMS Nov 20, 2020

AndyAyersMS left a comment

AndyAyersMS Nov 20, 2020

AndyAyersMS Nov 20, 2020

sandreenko Nov 20, 2020

AndyAyersMS Nov 23, 2020

AndyAyersMS Nov 20, 2020

AndyAyersMS Nov 20, 2020

sandreenko commented Nov 20, 2020

azure-pipelines bot commented Nov 20, 2020

AndyAyersMS left a comment

sandreenko commented Nov 25, 2020

sandreenko commented Nov 25, 2020


		if (corType != CORINFO_TYPE_CLASS && corType != CORINFO_TYPE_BYREF && corType != CORINFO_TYPE_PTR &&
		corType != CORINFO_TYPE_VAR && (argRealClass = info.compCompHnd->getArgClass(sig, argLst)) != nullptr)

Keep precise argument sizes between import and morph. #43130

Keep precise argument sizes between import and morph. #43130

Conversation

sandreenko commented Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Oct 12, 2020

AndyAyersMS commented Oct 12, 2020

sandreenko commented Oct 12, 2020

AndyAyersMS commented Oct 12, 2020

sandreenko commented Oct 12, 2020

CarolEidt 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

sandreenko commented Nov 19, 2020

azure-pipelines bot commented Nov 19, 2020

sandreenko commented Nov 20, 2020

azure-pipelines bot commented Nov 20, 2020

sandreenko 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

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

sandreenko commented Nov 20, 2020

azure-pipelines bot commented Nov 20, 2020

AndyAyersMS left a comment

Choose a reason for hiding this comment

sandreenko commented Nov 25, 2020

sandreenko commented Nov 25, 2020

sandreenko commented Oct 7, 2020 •

edited

Loading