Allow fasttail calls for engregesterable structs #6

jashook · 2017-06-26T16:56:31Z

Note

This causes 153 new fastTailCall decisions in System.Private.CoreLib. With no new decisions to not fastTailCall. See this list for new fastTailCalled methods.

briansull · 2017-06-26T17:34:15Z

src/jit/lclvars.cpp

+        varDsc->IsFloatRegType() ? ++floatingRegCount : ++argRegCount;
+    };
+
+    for (unsigned argNum = 0; argNum < info.compMethodInfo->args.numArgs; argNum++, varDscTrailingPointer++)


Consider this version of this loop used elsewhere:

for (lclNum = 0, varDsc = lvaTable; lclNum < lvaCount; lclNum++, varDsc++)

briansull · 2017-06-26T17:34:44Z

src/jit/lclvars.cpp

@@ -233,16 +240,55 @@ void Compiler::lvaInitTypeRef()
    InitVarDscInfo varDscInfo;
    varDscInfo.Init(lvaTable, hasRetBuffArg);

+    // Maintain a pointer to the start of the arguments.
+    LclVarDsc* varDscTrailingPointer = varDscInfo.varDsc;


renames as curArg or similiar

briansull · 2017-06-26T17:35:42Z

src/jit/lclvars.cpp

    // Finally the local variables
    //-------------------------------------------------------------------------

    unsigned                varNum    = varDscInfo.varNum;
    LclVarDsc*              varDsc    = varDscInfo.varDsc;
    CORINFO_ARG_LIST_HANDLE localsSig = info.compMethodInfo->locals.args;

+    // Maintain a pointer to the start of the locals.
+    varDscTrailingPointer = varDsc;


Is this a dead assignment?

briansull · 2017-06-26T17:41:33Z

src/jit/lclvars.cpp

@@ -255,13 +301,25 @@ void Compiler::lvaInitTypeRef()
        varDsc->lvPinned  = ((corInfoType & CORINFO_TYPE_MOD_PINNED) != 0);
        varDsc->lvOnFrame = true; // The final home for this local variable might be our local stack frame

+        CORINFO_CLASS_HANDLE clsHnd = info.compCompHnd->getArgClass(&info.compMethodInfo->locals, localsSig);
+        stackSize += roundUp(compGetTypeSize(strip(corInfoType), clsHnd), TARGET_POINTER_SIZE);


This local name is confusing: corInfoType

briansull · 2017-06-26T17:42:21Z

src/jit/morph.cpp

+    unsigned callerArgRegCount        = info.compArgRegCount;
+    unsigned callerFloatArgRegCount   = info.compFloatArgRegCount;
+
+    // TODO ARM64, UNIX x64


Need // TODO-ARM64/AMD64-UNIX instead

briansull · 2017-06-26T17:43:49Z

src/jit/morph.cpp

+                //
+                // hasMultiByteArgs will determine if the struct can be passed
+                // in registers. If it cannot we will break the loop and not
+                // fastTailCall.


I would prefer to have each line end with a period, NOTE is implied here

briansull · 2017-06-26T17:46:25Z

src/jit/morph.cpp


-                if (typeSize > TARGET_POINTER_SIZE)
+                // TODO. Here we have made the assumption that multibyte struct
+                // arguments will cause a no fastTailCall decision.


// TODO-AMD64-Linux

BruceForstall · 2017-06-26T18:42:58Z

In general, it would be useful to have a file-level comment for your new test cases, describing what they are intending to test -- an overall "here's why this is here" kind of comment.

Before this change structs on Arm64 and Amd64 Unix could pessimize when we could fastTailCall if they were engregisterable and took more than one register.

jashook · 2017-06-26T18:54:04Z

Ok this Pr is in a good state again. It has changes based on @briansull's comments that are incomplete.

BruceForstall · 2017-06-26T18:54:16Z

src/jit/lclvars.cpp

+    {
+        ++info.compArgsCount;
+        ++argRegCount;
+    };



This code assumes that anything using incrementArgCount (which is the four specials: 'this', retbuf, generics context, varargs cookie) are register arguments. But that doesn't appear to be true, e.g., for x86, which only has 2 register arguments. (There are also "non-standard" reg args for things like pinvoke helpers, etc.).

It seems like the only code that can count reg args correctly is fgMorphArgs, when AddRegArg() is called. I wonder if this calculation should be done then instead.

Ok, ignore that; I was confusing incoming and outgoing args.

BruceForstall · 2017-06-26T18:56:49Z

src/jit/lclvars.cpp

@@ -255,13 +301,25 @@ void Compiler::lvaInitTypeRef()
        varDsc->lvPinned  = ((corInfoType & CORINFO_TYPE_MOD_PINNED) != 0);
        varDsc->lvOnFrame = true; // The final home for this local variable might be our local stack frame

+        CORINFO_CLASS_HANDLE clsHnd = info.compCompHnd->getArgClass(&info.compMethodInfo->locals, localsSig);
+        stackSize += roundUp(compGetTypeSize(strip(corInfoType), clsHnd), TARGET_POINTER_SIZE);
+


You hoisted this out of the if (strip(corInfoType) == CORINFO_TYPE_CLASS) condition. Is it legal (or desirable) to call getArgClass in all cases?

BruceForstall · 2017-06-26T20:08:47Z

src/jit/lclvars.cpp

+    {
+        ++info.compArgsCount;
+        ++argRegCount;
+    };



Isn't this going to double-count the "specials"? Don't they have varDsc that is going to be counted by your new loop below the call to lvaInitArgs()?

BruceForstall · 2017-06-26T20:10:18Z

src/jit/lclvars.cpp

+            incrementRegCount(varDsc);
+
+#ifdef (FEATURE_MULTIREG_ARGS && !WINDOWS_AMD64_ABI)
+            if (varDsc->lvOtherArgReg != REG_NA)


Doesn't this need to be written:

#if defined(FEATURE_MULTIREG_ARGS) && !defined(WINDOWS_AMD64_ABI)

Instead of !defined(WINDOWS_AMD64_ABI) I would prefer defined(UNIX_AMD64_ABI)

In reply to: 124107782 [](ancestors = 124107782)

although it's not clear it needs any platform #ifdef. If it does, please add a comment as to why.

In reply to: 124108646 [](ancestors = 124108646,124107782)

BruceForstall · 2017-06-26T20:16:30Z

src/jit/lclvars.cpp

+    // Calculate the argument register usage.
+    //
+    // This will later be used for fastTailCall determination
+    //-------------------------------------------------------------------------


This whole section should probably be under #if FEATURE_FASTTAILCALL

BruceForstall · 2017-06-26T20:20:00Z

src/jit/lclvars.cpp

+    for (LclVarDsc* varDsc, unsigned argNum = 0; argNum < info.compMethodInfo->args.numArgs; argNum++, varDsc++)
+    {
+        if (varDsc->lvRegNum != REG_STK)
+        {


shouldn't this be lvArgReg? or maybe lvIsRegArg?

BruceForstall · 2017-06-26T20:23:32Z

src/jit/lclvars.cpp

+        else
+        {
+            stackSize += varDsc->lvSize();
+        }


lvSize() can only be used for structs. I think you need a different calculation here for argument stack size.

BruceForstall · 2017-06-26T20:31:15Z

src/jit/morph.cpp

+                }
+
+#else // ARM64
+                var_types hfaType  = GetHfaType(argx);


also Windows amd64, which I think you don't want?

BruceForstall · 2017-06-26T20:32:59Z

src/jit/morph.cpp

+    }
+
+#elif (_TARGET_AMD64_ && UNIX_AMD64_ABI) || _TARGET_ARM64_
+


I think this needs to be (or, at least, should be) written:

#elif (defined(_TARGET_AMD64_) && defined(UNIX_AMD64_ABI)) || defined(_TARGET_ARM64_)

BruceForstall · 2017-06-26T20:37:25Z

src/jit/morph.cpp

+
+    // For *nix Amd64 and Arm64 check to see if all arguments for the callee
+    // and caller are passing in registers. If not make sure that the stack
+    // size for the callee is at less than or equal to the caller's.


is at less [](start = 27, length = 10)

"... is at less ..."

Should the "at" be removed? I might rephrase to say:

If not, ensure that the outgoing argument stack size requirement for the callee is less than or equal to the caller's entire stack frame usage.

(I assume we care about the caller's entire stack frame usage, not the caller's argument stack space usage, which would be less.)

BruceForstall · 2017-06-26T20:40:56Z

src/jit/morph.cpp

+    // x64 Windows: If we have more callee registers used than MAX_REG_ARG, then 
+    // make sure the callee's incoming arguments is less than the caller's
+    if ((((calleeArgRegCount + calleeFloatArgRegCount) > maxRegArgs) && 
+        ((callerArgRegCount + callerFloatArgRegCount) > (callerArgRegCount + callerFloatArgRegCount)))


(callerArgRegCount + callerFloatArgRegCount) > (callerArgRegCount + callerFloatArgRegCount) [](start = 9, length = 91)

There's a problem here:

(callerArgRegCount + callerFloatArgRegCount) > (callerArgRegCount + callerFloatArgRegCount)

looks like a > a

LowerFastTailCall assumes nCallerArgs == nCalleeArgs. This is tracked with https://github.com/dotnet/coreclr/issues/12468. It should be fixed in a seperate PR.

Now shows no diff in fastTailCall decisions on Amd64 Windows.

briansull · 2017-06-27T20:41:52Z

src/jit/morph.cpp

-#endif // FEATURE_UNIX_AMD64_STRUCT_PASSING || _TARGET_ARM64_
+                else
+                {
+                    if (structDesc.eightByteCount > 1)


This reads better as:

if (structDesc.eightByteCount == 2)

briansull

Approved with one comment

# This is the 1st commit message: Remove CoreFX runtest dependency This change will start using build-test.sh generatelayoutonly to build the coreoverlay directory for use with runtest.sh. # This is the commit message #2: Missing ./ # This is the commit message #3: Fix untar location # This is the commit message #4: Use portable builds # This is the commit message #5: Get ci green # This is the commit message #6: Small changes # This is the commit message #7: Small changes # This is the commit message #8: Small changes # This is the commit message #9: Add generateonly builds.sh # This is the commit message #10: Ensure correct test location # This is the commit message #11: netci change # This is the commit message #12: netci change # This is the commit message #13: Working on setting up build-test # This is the commit message #14: Remove generatelayoutonly # This is the commit message #15: Undo some of the changes to use generatelayoutonly # This is the commit message #16: Correctly remove build-test.sh invocation # This is the commit message #17: Fix build-test generatelayoutonly # This is the commit message #18: Fix a few netci issues

briansull reviewed Jun 26, 2017

View reviewed changes

jashook force-pushed the allow_fasttail_calls_for_engregesterable_structs branch 2 times, most recently from ad4f1b1 to b7590e3 Compare June 26, 2017 18:47

[*64] Allow more fastTailCalls involving structs

3b1a01d

Before this change structs on Arm64 and Amd64 Unix could pessimize when we could fastTailCall if they were engregisterable and took more than one register.

jashook force-pushed the allow_fasttail_calls_for_engregesterable_structs branch from b7590e3 to 3b1a01d Compare June 26, 2017 18:53

BruceForstall reviewed Jun 26, 2017

View reviewed changes

Exclude stackBound two slot size structs from fastTailCalls

6b4e1b8

BruceForstall reviewed Jun 26, 2017

View reviewed changes

src/jit/morph.cpp Outdated

}

#else // ARM64

var_types hfaType = GetHfaType(argx);

Copy link

BruceForstall Jun 26, 2017

Choose a reason for hiding this comment

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

also Windows amd64, which I think you don't want?

BruceForstall reviewed Jun 26, 2017

View reviewed changes

jashook added 6 commits June 26, 2017 14:34

Address feedback

0f34537

Fix a > a

facad52

Fix log message

faef47c

Requiring nCalleeArgs==nCallerArgs if hasStackArgs

55cc1b1

LowerFastTailCall assumes nCallerArgs == nCalleeArgs. This is tracked with https://github.com/dotnet/coreclr/issues/12468. It should be fixed in a seperate PR.

Remove debug code.

3e48cf4

Fix windows build breaks

01435c4

jashook added 4 commits June 27, 2017 10:16

Fix windows warnings

cfea02f

Fix last windows warning

f13b6cd

Fix hopefully the last windows warning

b1370ad

Make the correct fastTailCall decision on Windows

56d267e

Now shows no diff in fastTailCall decisions on Amd64 Windows.

jashook closed this Jun 27, 2017

briansull reviewed Jun 27, 2017

View reviewed changes

Allow fasttail calls for engregesterable structs #6

Allow fasttail calls for engregesterable structs #6

Uh oh!

Conversation

jashook commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Jun 26, 2017

Uh oh!

jashook commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jashook commented Jun 26, 2017 •

edited

Loading

briansull Jun 26, 2017 •

edited

Loading

briansull Jun 26, 2017 •

edited

Loading