Address P/Invoke inlining code review feedback and cleanup

Address some code review feedback from #5939. Also, do a little cleanup.
Specifically:
1. Make many GenTreeCall accessors 'const'.
2. HasNonStandardArgs() and GetNonStandardArgCount() are used for a specific purpose in the fast
tailcall implementation. However, the fgMorphArgs() "non-standard args" mechanism is now used for
other purposes, so the names of these didn't match their intent. Also, the logic had decayed compared
to the fgMorphArgs logic it was mimicing. I renamed them to add the word "Added", and wrote some
comments to be more explicit about their use, as well as indicate in fgMorphArgs() that they need
to be kept in sync.
3. Added several GenTreeCall::IsHelperCall() accessors, and replace some code with them.
4. Added RBM_INIT_PINVOKE_FRAME_TRASH define to remove an `#ifdef`.
5. Removed an assert loop in LowerFastTailCall() since it's no longer accurate given the new users of `isNonStandard`.

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