Merge pull request #6038 from BruceForstall/PinvokeFeedbackCleanup

Address P/Invoke inlining code review feedback and cleanup

Author: BruceForstall

src/jit/codegencommon.cpp

@@ -686,12 +686,8 @@ regMaskTP Compiler::compHelperCallKillSet(CorInfoHelpFunc helper)
     case CORINFO_HELP_STOP_FOR_GC:
         return RBM_STOP_FOR_GC_TRASH;
 
-#ifdef _TARGET_X86_
     case CORINFO_HELP_INIT_PINVOKE_FRAME:
-        // On x86, this helper has a custom calling convention that takes EDI as argument
-        // (but doesn't trash it), trashes EAX, and returns ESI.
-        return RBM_PINVOKE_SCRATCH | RBM_PINVOKE_TCB;
-#endif // _TARGET_X86_
+        return RBM_INIT_PINVOKE_FRAME_TRASH;
 
     default:
         return RBM_CALLEE_TRASH;

src/jit/codegenxarch.cpp

@@ -6064,7 +6064,7 @@ void CodeGen::genCallInstruction(GenTreePtr node)
             else
             {                
 #ifdef _TARGET_X86_
-                if ((call->gtCallType == CT_HELPER) && (call->gtCallMethHnd == compiler->eeFindHelper(CORINFO_HELP_INIT_PINVOKE_FRAME)))
+                if (call->IsHelperCall(compiler, CORINFO_HELP_INIT_PINVOKE_FRAME))
                 {
                     // The x86 CORINFO_HELP_INIT_PINVOKE_FRAME helper uses a custom calling convention that returns with
                     // TCB in REG_PINVOKE_TCB. AMD64/ARM64 use the standard calling convention. fgMorphCall() sets the

src/jit/gentree.cpp

@@ -1349,6 +1349,71 @@ GenTreeCall::GetOtherRegMask() const
     return resultMask;
 }
 
+#ifndef LEGACY_BACKEND
+
+//-------------------------------------------------------------------------
+// HasNonStandardAddedArgs: Return true if the method has non-standard args added to the call
+// argument list during argument morphing (fgMorphArgs), e.g., passed in R10 or R11 on AMD64.
+// See also GetNonStandardAddedArgCount().
+//
+// Arguments:
+//     compiler - the compiler instance
+//
+// Return Value:
+//      true if there are any such args, false otherwise.
+//
+bool GenTreeCall::HasNonStandardAddedArgs(Compiler* compiler) const
+{
+    return GetNonStandardAddedArgCount(compiler) != 0;
+}
+
+
+//-------------------------------------------------------------------------
+// GetNonStandardAddedArgCount: Get the count of non-standard arguments that have been added
+// during call argument morphing (fgMorphArgs). Do not count non-standard args that are already
+// counted in the argument list prior to morphing.
+//
+// This function is used to help map the caller and callee arguments during tail call setup.
+//
+// Arguments:
+//     compiler - the compiler instance
+//
+// Return Value:
+//      The count of args, as described.
+//
+// Notes:
+//      It would be more general to have fgMorphArgs set a bit on the call node when such
+//      args are added to a call, and a bit on each such arg, and then have this code loop
+//      over the call args when the special call bit is set, counting the args with the special
+//      arg bit. This seems pretty heavyweight, though. Instead, this logic needs to be kept
+//      in sync with fgMorphArgs.
+//
+int GenTreeCall::GetNonStandardAddedArgCount(Compiler* compiler) const
+{
+    if (IsUnmanaged() && !compiler->opts.ShouldUsePInvokeHelpers())
+    {
+        // R11 = PInvoke cookie param
+        return 1;
+    }
+    else if (gtCallType == CT_INDIRECT)
+    {
+        if (IsVirtualStub())
+        {
+            // R11 = Virtual stub param
+            return 1;
+        }
+        else if (gtCallCookie != nullptr)
+        {
+            // R10 = PInvoke target param
+            // R11 = PInvoke cookie param
+            return 2;
+        }
+    } 
+    return 0;
+}
+
+#endif // !LEGACY_BACKEND
+
 //-------------------------------------------------------------------------
 // TreatAsHasRetBufArg:
 //
@@ -1370,7 +1435,7 @@ GenTreeCall::GetOtherRegMask() const
 //     aren't actually defined to return a struct, so they don't expect
 //     their RetBuf to be passed in x8, instead they  expect it in x0.
 //
-bool GenTreeCall::TreatAsHasRetBufArg(Compiler* compiler)
+bool GenTreeCall::TreatAsHasRetBufArg(Compiler* compiler) const
 {
     if (HasRetBufArg())
     {
@@ -1381,12 +1446,12 @@ bool GenTreeCall::TreatAsHasRetBufArg(Compiler* compiler)
         // If we see a Jit helper call that returns a TYP_STRUCT we will
         // transform it as if it has a Return Buffer Argument
         //
-        if (IsHelperCall() && (gtCall.gtReturnType == TYP_STRUCT))
+        if (IsHelperCall() && (gtReturnType == TYP_STRUCT))
         {
             // There are two possible helper calls that use this path:
             //  CORINFO_HELP_GETFIELDSTRUCT and CORINFO_HELP_UNBOX_NULLABLE
             //
-            CorInfoHelpFunc helpFunc = compiler->eeGetHelperNum(gtCall.gtCallMethHnd);
+            CorInfoHelpFunc helpFunc = compiler->eeGetHelperNum(gtCallMethHnd);
 
             if (helpFunc == CORINFO_HELP_GETFIELDSTRUCT)
             {
@@ -1404,6 +1469,22 @@ bool GenTreeCall::TreatAsHasRetBufArg(Compiler* compiler)
     }
     return false;
 }
+ 
+
+//-------------------------------------------------------------------------
+// IsHelperCall: Determine if this GT_CALL node is a specific helper call.
+//
+// Arguments:
+//     compiler - the compiler instance so that we can call eeFindHelper
+//
+// Return Value:
+//     Returns true if this GT_CALL node is a call to the specified helper.
+//
+bool GenTreeCall::IsHelperCall(Compiler* compiler, unsigned helper) const
+{
+    return IsHelperCall(compiler->eeFindHelper(helper));
+}
+
 
 /*****************************************************************************
  *

src/jit/gentree.h

@@ -2767,44 +2767,17 @@ struct GenTreeCall final : public GenTree
 
 #define     GTF_CALL_M_R2R_REL_INDIRECT        0x2000  // GT_CALL -- ready to run call is indirected through a relative address
 
-    bool IsUnmanaged()       { return (gtFlags & GTF_CALL_UNMANAGED) != 0; }
-    bool NeedsNullCheck()    { return (gtFlags & GTF_CALL_NULLCHECK) != 0; }
-    bool CallerPop()         { return (gtFlags & GTF_CALL_POP_ARGS) != 0;  }
-    bool IsVirtual()         { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) != GTF_CALL_NONVIRT; }
-    bool IsVirtualStub()     { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) == GTF_CALL_VIRT_STUB; }
-    bool IsVirtualVtable()   { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) == GTF_CALL_VIRT_VTABLE; }
-    bool IsInlineCandidate() { return (gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0; }
+    bool IsUnmanaged()       const { return (gtFlags & GTF_CALL_UNMANAGED) != 0; }
+    bool NeedsNullCheck()    const { return (gtFlags & GTF_CALL_NULLCHECK) != 0; }
+    bool CallerPop()         const { return (gtFlags & GTF_CALL_POP_ARGS) != 0;  }
+    bool IsVirtual()         const { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) != GTF_CALL_NONVIRT; }
+    bool IsVirtualStub()     const { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) == GTF_CALL_VIRT_STUB; }
+    bool IsVirtualVtable()   const { return (gtFlags & GTF_CALL_VIRT_KIND_MASK) == GTF_CALL_VIRT_VTABLE; }
+    bool IsInlineCandidate() const { return (gtFlags & GTF_CALL_INLINE_CANDIDATE) != 0; }
 
 #ifndef LEGACY_BACKEND
-    // Whether the method has non-standard args (i.e. passed in R10 or R11)
-    // See fgMorphArgs() to know the call types for which non-standard args are inserted.
-    bool HasNonStandardArgs()  { return IsUnmanaged() || (gtCallType == CT_INDIRECT && (IsVirtualStub() || gtCallCookie)); }
-
-    // Get the count of non-standard arg count 
-    int  GetNonStandardArgCount() 
-    {
-        if (IsUnmanaged())
-        {
-            // R11 = PInvoke cookie param
-            return 1;
-        }
-        else if (gtCallType == CT_INDIRECT)
-        {
-            if (IsVirtualStub())
-            {
-                // R11 = Virtual stub param
-                return 1;
-            }
-            else if (gtCallCookie != nullptr)
-            {
-                // R10 = PInvoke target param
-                // R11 = PInvoke cookie param
-                return 2;
-            }
-        }
-
-        return 0;
-    }
+    bool HasNonStandardAddedArgs(Compiler* compiler) const;
+    int GetNonStandardAddedArgCount(Compiler* compiler) const;
 #endif // !LEGACY_BACKEND
 
     // Returns true if this call uses a retBuf argument and its calling convention
@@ -2830,7 +2803,7 @@ struct GenTreeCall final : public GenTree
     //     will make HasRetBufArg() return true, but will also force the 
     //     use of register x8 to pass the RetBuf argument.
     //
-    bool TreatAsHasRetBufArg(Compiler* compiler);
+    bool TreatAsHasRetBufArg(Compiler* compiler) const;
 
     //-----------------------------------------------------------------------------------------
     // HasMultiRegRetVal: whether the call node returns its value in multiple return registers.
@@ -2859,57 +2832,60 @@ struct GenTreeCall final : public GenTree
     }
 
     // Returns true if VM has flagged this method as CORINFO_FLG_PINVOKE.
-    bool IsPInvoke()                { return (gtCallMoreFlags & GTF_CALL_M_PINVOKE) != 0; }
+    bool IsPInvoke() const              { return (gtCallMoreFlags & GTF_CALL_M_PINVOKE) != 0; }
 
     // Note that the distinction of whether tail prefixed or an implicit tail call
     // is maintained on a call node till fgMorphCall() after which it will be
     // either a tail call (i.e. IsTailCall() is true) or a non-tail call.
-    bool IsTailPrefixedCall()       { return (gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; }     
+    bool IsTailPrefixedCall() const     { return (gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; }     
 
     // This method returning "true" implies that tail call flowgraph morhphing has 
     // performed final checks and committed to making a tail call.
-    bool IsTailCall()               { return (gtCallMoreFlags & GTF_CALL_M_TAILCALL) != 0; }
+    bool IsTailCall() const             { return (gtCallMoreFlags & GTF_CALL_M_TAILCALL) != 0; }
 
     // This method returning "true" implies that importer has performed tail call checks
     // and providing a hint that this can be converted to a tail call.
-    bool CanTailCall()              { return IsTailPrefixedCall() || IsImplicitTailCall(); }
+    bool CanTailCall() const            { return IsTailPrefixedCall() || IsImplicitTailCall(); }
 
 #ifndef LEGACY_BACKEND
-    bool IsTailCallViaHelper()      { return IsTailCall() && (gtCallMoreFlags & GTF_CALL_M_TAILCALL_VIA_HELPER); } 
+    bool IsTailCallViaHelper() const    { return IsTailCall() && (gtCallMoreFlags & GTF_CALL_M_TAILCALL_VIA_HELPER); } 
 #else // LEGACY_BACKEND
-    bool IsTailCallViaHelper()      { return true; }
+    bool IsTailCallViaHelper() const    { return true; }
 #endif // LEGACY_BACKEND
 
 #if FEATURE_FASTTAILCALL    
-    bool IsFastTailCall()           { return IsTailCall() && !(gtCallMoreFlags & GTF_CALL_M_TAILCALL_VIA_HELPER); }
+    bool IsFastTailCall() const         { return IsTailCall() && !(gtCallMoreFlags & GTF_CALL_M_TAILCALL_VIA_HELPER); }
 #else // !FEATURE_FASTTAILCALL
-    bool IsFastTailCall()           { return false; }    
+    bool IsFastTailCall() const         { return false; }    
 #endif // !FEATURE_FASTTAILCALL
 
 #if FEATURE_TAILCALL_OPT
     // Returns true if this is marked for opportunistic tail calling.
     // That is, can be tail called though not explicitly prefixed with "tail" prefix.
-    bool IsImplicitTailCall()       { return (gtCallMoreFlags & GTF_CALL_M_IMPLICIT_TAILCALL) != 0; }
-    bool IsTailCallConvertibleToLoop() { return (gtCallMoreFlags & GTF_CALL_M_TAILCALL_TO_LOOP) != 0; }
+    bool IsImplicitTailCall() const             { return (gtCallMoreFlags & GTF_CALL_M_IMPLICIT_TAILCALL) != 0; }
+    bool IsTailCallConvertibleToLoop() const    { return (gtCallMoreFlags & GTF_CALL_M_TAILCALL_TO_LOOP) != 0; }
 #else // !FEATURE_TAILCALL_OPT
-    bool IsImplicitTailCall()       { return false; }
-    bool IsTailCallConvertibleToLoop() { return false; }
+    bool IsImplicitTailCall() const             { return false; }
+    bool IsTailCallConvertibleToLoop() const    { return false; }
 #endif // !FEATURE_TAILCALL_OPT
 
-    bool IsSameThis()      { return (gtCallMoreFlags & GTF_CALL_M_NONVIRT_SAME_THIS) != 0; } 
-    bool IsDelegateInvoke(){ return (gtCallMoreFlags & GTF_CALL_M_DELEGATE_INV) != 0; } 
-    bool IsVirtualStubRelativeIndir() { return (gtCallMoreFlags & GTF_CALL_M_VIRTSTUB_REL_INDIRECT) != 0; } 
+    bool IsSameThis() const                 { return (gtCallMoreFlags & GTF_CALL_M_NONVIRT_SAME_THIS) != 0; } 
+    bool IsDelegateInvoke() const           { return (gtCallMoreFlags & GTF_CALL_M_DELEGATE_INV) != 0; } 
+    bool IsVirtualStubRelativeIndir() const { return (gtCallMoreFlags & GTF_CALL_M_VIRTSTUB_REL_INDIRECT) != 0; } 
+
 #ifdef FEATURE_READYTORUN_COMPILER
-    bool IsR2RRelativeIndir() { return (gtCallMoreFlags & GTF_CALL_M_R2R_REL_INDIRECT) != 0; }
-    void setEntryPoint(CORINFO_CONST_LOOKUP entryPoint) {
+    bool IsR2RRelativeIndir() const         { return (gtCallMoreFlags & GTF_CALL_M_R2R_REL_INDIRECT) != 0; }
+    void setEntryPoint(CORINFO_CONST_LOOKUP entryPoint)
+    {
         gtEntryPoint = entryPoint;
         if (gtEntryPoint.accessType == IAT_PVALUE)
         {
             gtCallMoreFlags |= GTF_CALL_M_R2R_REL_INDIRECT;
         }
     }
 #endif // FEATURE_READYTORUN_COMPILER
-    bool IsVarargs()       { return (gtCallMoreFlags & GTF_CALL_M_VARARGS) != 0; }
+
+    bool IsVarargs() const                  { return (gtCallMoreFlags & GTF_CALL_M_VARARGS) != 0; }
 
     unsigned short  gtCallMoreFlags;        // in addition to gtFlags
 
@@ -2951,6 +2927,18 @@ struct GenTreeCall final : public GenTree
     // IL offset of the call wrt its parent method.
     IL_OFFSET gtRawILOffset;
 #endif // defined(DEBUG) || defined(INLINE_DATA)
+ 
+    bool IsHelperCall() const
+    {
+        return gtCallType == CT_HELPER;
+    }
+ 
+    bool IsHelperCall(CORINFO_METHOD_HANDLE callMethHnd) const
+    {
+        return IsHelperCall() && (callMethHnd == gtCallMethHnd);
+    }
+ 
+    bool IsHelperCall(Compiler* compiler, unsigned helper) const;
 
     GenTreeCall(var_types type) : 
         GenTree(GT_CALL, type) 

src/jit/lower.cpp

@@ -2450,26 +2450,7 @@ void Lowering::LowerFastTailCall(GenTreeCall *call)
     // accounted in caller's arg count but accounted in callee's arg count after 
     // fgMorphArgs(). Therefore, exclude callee's non-standard args while mapping
     // callee's stack arg num to corresponding caller's stack arg num.
-    unsigned calleeNonStandardArgCount = call->GetNonStandardArgCount();
-
-#ifdef DEBUG
-    // cross check non-standard arg count.
-    if (firstPutArgStk && call->HasNonStandardArgs())
-    {
-        fgArgInfoPtr         argInfo = call->gtCall.fgArgInfo;
-        unsigned            argCount = argInfo->ArgCount();
-        fgArgTabEntryPtr *  argTable = argInfo->ArgTable();
-
-        unsigned cnt = 0;
-        for (unsigned i=0; i < argCount; i++)
-        {
-            if (argTable[i]->isNonStandard)
-                ++cnt;
-        }
-        assert(cnt == calleeNonStandardArgCount);
-    }
-#endif
-
+    unsigned calleeNonStandardArgCount = call->GetNonStandardAddedArgCount(comp);
 
     // Say Caller(a, b, c, d, e) fast tail calls Callee(e, d, c, b, a)
     // i.e. passes its arguments in reverse to Callee. During call site
@@ -3135,7 +3116,7 @@ void Lowering::InsertPInvokeMethodProlog()
     GenTree* lastStmt = stmt;
     DISPTREE(lastStmt);
 
-#ifndef _TARGET_X86_ // For x86, this step is done at the call site.
+#ifndef _TARGET_X86_ // For x86, this step is done at the call site (due to stack pointer not being static in the function).
 
     // --------------------------------------------------------
     // InlinedCallFrame.m_pCallSiteSP = @RSP;

src/jit/lowerxarch.cpp

@@ -1073,7 +1073,7 @@ Lowering::TreeNodeInfoInitCall(GenTreeCall* call)
 
     // Set destination candidates for return value of the call.
 #ifdef _TARGET_X86_
-    if ((call->gtCallType == CT_HELPER) && (call->gtCallMethHnd == compiler->eeFindHelper(CORINFO_HELP_INIT_PINVOKE_FRAME)))
+    if (call->IsHelperCall(compiler, CORINFO_HELP_INIT_PINVOKE_FRAME))
     {
         // The x86 CORINFO_HELP_INIT_PINVOKE_FRAME helper uses a custom calling convention that returns with
         // TCB in REG_PINVOKE_TCB. AMD64/ARM64 use the standard calling convention. fgMorphCall() sets the