Runtime lookup clean up, enable for helper-based tail calls

src/coreclr/inc/corinfo.h

@@ -1268,9 +1268,6 @@ struct CORINFO_RUNTIME_LOOKUP
     // If set, test for null and branch to helper if null
     bool                    testForNull;
 
-    // If set, test the lowest bit and dereference if set (see code:FixupPointer)
-    bool                    testForFixup;
-
     uint16_t                sizeOffset;
     size_t                  offsets[CORINFO_MAXINDIRECTIONS];
 

src/coreclr/inc/jiteeversionguid.h

@@ -43,11 +43,11 @@ typedef const GUID *LPCGUID;
 #define GUID_DEFINED
 #endif // !GUID_DEFINED
 
-constexpr GUID JITEEVersionIdentifier = { /* b0e1ce41-2339-491d-ab72-736a8d233ea1 */
-    0xb0e1ce41,
-    0x2339,
-    0x491d,
-    {0xab, 0x72, 0x73, 0x6a, 0x8d, 0x23, 0x3e, 0xa1}
+constexpr GUID JITEEVersionIdentifier = { /* 3a8a07e7-928e-4281-ab68-cd4017c1141b */
+    0x3a8a07e7,
+    0x928e,
+    0x4281,
+    {0xab, 0x68, 0xcd, 0x40, 0x17, 0xc1, 0x14, 0x1b}
   };
 
 //////////////////////////////////////////////////////////////////////////////////////////////////////////

src/coreclr/jit/compiler.hpp

@@ -1029,27 +1029,6 @@ inline GenTreeCall* Compiler::gtNewHelperCallNode(
     return result;
 }
 
-//------------------------------------------------------------------------------
-// gtNewRuntimeLookupHelperCallNode : Helper to create a runtime lookup call helper node.
-//
-//
-// Arguments:
-//    helper    - Call helper
-//    type      - Type of the node
-//    args      - Call args
-//
-// Return Value:
-//    New CT_HELPER node
-
-inline GenTreeCall* Compiler::gtNewRuntimeLookupHelperCallNode(CORINFO_RUNTIME_LOOKUP* pRuntimeLookup,
-                                                               GenTree*                ctxTree,
-                                                               void*                   compileTimeHandle)
-{
-    GenTree* argNode  = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_GLOBAL_PTR, compileTimeHandle);
-    GenTreeCall* call = gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, ctxTree, argNode);
-    return call;
-}
-
 //------------------------------------------------------------------------
 // gtNewAllocObjNode: A little helper to create an object allocation node.
 //

src/coreclr/jit/fgbasic.cpp

@@ -4643,6 +4643,9 @@ BasicBlock* Compiler::fgSplitBlockBeforeTree(
     BasicBlockFlags originalFlags = block->bbFlags;
     BasicBlock*     prevBb        = block;
 
+    // We use fgSplitBlockAfterStatement() API here to split the block, however, we want to split
+    // it *Before* rather than *After* so if the current statement is the first in the
+    // current block - invoke fgSplitBlockAtBeginning
     if (stmt == block->firstStmt())
     {
         block = fgSplitBlockAtBeginning(prevBb);

src/coreclr/jit/importer.cpp

@@ -1782,36 +1782,22 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken
 
     if (pRuntimeLookup->testForNull)
     {
+        // Import just a helper call and mark it for late expansion in fgExpandRuntimeLookups phase
         assert(pRuntimeLookup->indirections != 0);
+        GenTreeCall* helperCall = gtNewRuntimeLookupHelperCallNode(pRuntimeLookup, ctxTree, compileTimeHandle);
 
-        // Call the helper
-        // - Setup argNode with the pointer to the signature returned by the lookup
-        GenTree* argNode =
-            gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_GLOBAL_PTR, compileTimeHandle);
-        GenTreeCall* helperCall = gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, ctxTree, argNode);
-
-        // No need to perform CSE/hoisting for signature node - it is expected to end up in a rarely-taken block after
-        // "Expand runtime lookups" phase.
-        argNode->gtFlags |= GTF_DONT_CSE;
-
-        // Leave a note that this method has runtime lookups we might want to expand (nullchecks, size checks) later.
-        // We can also consider marking current block as a runtime lookup holder to improve TP for Tier0
-        impInlineRoot()->setMethodHasExpRuntimeLookup();
-        helperCall->SetExpRuntimeLookup();
-        if (!impInlineRoot()->GetSignatureToLookupInfoMap()->Lookup(pRuntimeLookup->signature))
-        {
-            JITDUMP("Registering %p in SignatureToLookupInfoMap\n", pRuntimeLookup->signature)
-            impInlineRoot()->GetSignatureToLookupInfoMap()->Set(pRuntimeLookup->signature, *pRuntimeLookup);
-        }
+        // Spilling it to a temp improves CQ (mainly in Tier0)
         unsigned callLclNum = lvaGrabTemp(true DEBUGARG("spilling helperCall"));
         impAssignTempGen(callLclNum, helperCall);
         return gtNewLclvNode(callLclNum, helperCall->TypeGet());
     }
 
+    // Size-check is not expected without testForNull
+    assert(pRuntimeLookup->sizeOffset == CORINFO_NO_SIZE_CHECK);
+
     // Slot pointer
-    GenTree* slotPtrTree   = ctxTree;
-    GenTree* indOffTree    = nullptr;
-    GenTree* lastIndOfTree = nullptr;
+    GenTree* slotPtrTree = ctxTree;
+    GenTree* indOffTree  = nullptr;
 
     // Applied repeated indirections
     for (WORD i = 0; i < pRuntimeLookup->indirections; i++)
@@ -1822,18 +1808,10 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken
                                       nullptr DEBUGARG("impRuntimeLookup indirectOffset"));
         }
 
-        // The last indirection could be subject to a size check (dynamic dictionary expansion)
-        bool isLastIndirectionWithSizeCheck =
-            ((i == pRuntimeLookup->indirections - 1) && (pRuntimeLookup->sizeOffset != CORINFO_NO_SIZE_CHECK));
-
         if (i != 0)
         {
             slotPtrTree = gtNewOperNode(GT_IND, TYP_I_IMPL, slotPtrTree);
-            slotPtrTree->gtFlags |= GTF_IND_NONFAULTING;
-            if (!isLastIndirectionWithSizeCheck)
-            {
-                slotPtrTree->gtFlags |= GTF_IND_INVARIANT;
-            }
+            slotPtrTree->gtFlags |= (GTF_IND_NONFAULTING | GTF_IND_INVARIANT);
         }
 
         if ((i == 1 && pRuntimeLookup->indirectFirstOffset) || (i == 2 && pRuntimeLookup->indirectSecondOffset))
@@ -1843,12 +1821,6 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken
 
         if (pRuntimeLookup->offsets[i] != 0)
         {
-            if (isLastIndirectionWithSizeCheck)
-            {
-                lastIndOfTree = impCloneExpr(slotPtrTree, &slotPtrTree, NO_CLASS_HANDLE, CHECK_SPILL_ALL,
-                                             nullptr DEBUGARG("impRuntimeLookup indirectOffset"));
-            }
-
             slotPtrTree =
                 gtNewOperNode(GT_ADD, TYP_I_IMPL, slotPtrTree, gtNewIconNode(pRuntimeLookup->offsets[i], TYP_I_IMPL));
         }
@@ -1865,37 +1837,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken
     slotPtrTree = gtNewOperNode(GT_IND, TYP_I_IMPL, slotPtrTree);
     slotPtrTree->gtFlags |= GTF_IND_NONFAULTING;
 
-    if (!pRuntimeLookup->testForFixup)
-    {
-        return slotPtrTree;
-    }
-
-    impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("bubbling QMark0"));
-
-    unsigned slotLclNum = lvaGrabTemp(true DEBUGARG("impRuntimeLookup test"));
-    impAssignTempGen(slotLclNum, slotPtrTree, NO_CLASS_HANDLE, CHECK_SPILL_ALL, nullptr, impCurStmtDI);
-
-    GenTree* slot = gtNewLclvNode(slotLclNum, TYP_I_IMPL);
-    // downcast the pointer to a TYP_INT on 64-bit targets
-    slot = impImplicitIorI4Cast(slot, TYP_INT);
-    // Use a GT_AND to check for the lowest bit and indirect if it is set
-    GenTree* test  = gtNewOperNode(GT_AND, TYP_INT, slot, gtNewIconNode(1));
-    GenTree* relop = gtNewOperNode(GT_EQ, TYP_INT, test, gtNewIconNode(0));
-
-    // slot = GT_IND(slot - 1)
-    slot           = gtNewLclvNode(slotLclNum, TYP_I_IMPL);
-    GenTree* add   = gtNewOperNode(GT_ADD, TYP_I_IMPL, slot, gtNewIconNode(-1, TYP_I_IMPL));
-    GenTree* indir = gtNewOperNode(GT_IND, TYP_I_IMPL, add);
-    indir->gtFlags |= GTF_IND_NONFAULTING;
-    indir->gtFlags |= GTF_IND_INVARIANT;
-
-    slot                = gtNewLclvNode(slotLclNum, TYP_I_IMPL);
-    GenTree*      asg   = gtNewAssignNode(slot, indir);
-    GenTreeColon* colon = new (this, GT_COLON) GenTreeColon(TYP_VOID, gtNewNothingNode(), asg);
-    GenTreeQmark* qmark = gtNewQmarkNode(TYP_VOID, relop, colon);
-    impAppendTree(qmark, CHECK_SPILL_NONE, impCurStmtDI);
-
-    return gtNewLclvNode(slotLclNum, TYP_I_IMPL);
+    return slotPtrTree;
 }
 
 struct RecursiveGuard

src/coreclr/jit/morph.cpp

@@ -7047,14 +7047,8 @@ GenTree* Compiler::getRuntimeLookupTree(CORINFO_RESOLVED_TOKEN* pResolvedToken,
     // If pRuntimeLookup->indirections is equal to CORINFO_USEHELPER, it specifies that a run-time helper should be
     // used; otherwise, it specifies the number of indirections via pRuntimeLookup->offsets array.
     if ((pRuntimeLookup->indirections == CORINFO_USEHELPER) || (pRuntimeLookup->indirections == CORINFO_USENULL) ||
-        pRuntimeLookup->testForNull || pRuntimeLookup->testForFixup)
-    {
-        // If the first condition is true, runtime lookup tree is available only via the run-time helper function.
-        // TODO-CQ If the second or third condition is true, we are always using the slow path since we can't
-        // introduce control flow at this point. See impRuntimeLookupToTree for the logic to avoid calling the helper.
-        // The long-term solution is to introduce a new node representing a runtime lookup, create instances
-        // of that node both in the importer and here, and expand the node in lower (introducing control flow if
-        // necessary).
+        pRuntimeLookup->testForNull)
+    {
         return gtNewRuntimeLookupHelperCallNode(pRuntimeLookup,
                                                 getRuntimeContextTree(pLookup->lookupKind.runtimeLookupKind),
                                                 compileTimeHandle);
@@ -7111,7 +7105,6 @@ GenTree* Compiler::getRuntimeLookupTree(CORINFO_RESOLVED_TOKEN* pResolvedToken,
     assert(!pRuntimeLookup->testForNull);
     if (pRuntimeLookup->indirections > 0)
     {
-        assert(!pRuntimeLookup->testForFixup);
         result = gtNewOperNode(GT_IND, TYP_I_IMPL, result);
         result->gtFlags |= GTF_IND_NONFAULTING;
     }

src/coreclr/jit/runtimelookup.cpp

@@ -53,6 +53,42 @@ static BasicBlock* CreateBlockFromTree(Compiler*   comp,
     return newBlock;
 }
 
+//------------------------------------------------------------------------------
+// gtNewRuntimeLookupHelperCallNode : Helper to create a runtime lookup call helper node.
+//
+// Arguments:
+//    helper    - Call helper
+//    type      - Type of the node
+//    args      - Call args
+//
+// Return Value:
+//    New CT_HELPER node
+//
+GenTreeCall* Compiler::gtNewRuntimeLookupHelperCallNode(CORINFO_RUNTIME_LOOKUP* pRuntimeLookup,
+                                                        GenTree*                ctxTree,
+                                                        void*                   compileTimeHandle)
+{
+    // Call the helper
+    // - Setup argNode with the pointer to the signature returned by the lookup
+    GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_GLOBAL_PTR, compileTimeHandle);
+    GenTreeCall* helperCall = gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, ctxTree, argNode);
+
+    // No need to perform CSE/hoisting for signature node - it is expected to end up in a rarely-taken block after
+    // "Expand runtime lookups" phase.
+    argNode->gtFlags |= GTF_DONT_CSE;
+
+    // Leave a note that this method has runtime lookups we might want to expand (nullchecks, size checks) later.
+    // We can also consider marking current block as a runtime lookup holder to improve TP for Tier0
+    impInlineRoot()->setMethodHasExpRuntimeLookup();
+    helperCall->SetExpRuntimeLookup();
+    if (!impInlineRoot()->GetSignatureToLookupInfoMap()->Lookup(pRuntimeLookup->signature))
+    {
+        JITDUMP("Registering %p in SignatureToLookupInfoMap\n", pRuntimeLookup->signature)
+        impInlineRoot()->GetSignatureToLookupInfoMap()->Set(pRuntimeLookup->signature, *pRuntimeLookup);
+    }
+    return helperCall;
+}
+
 //------------------------------------------------------------------------------
 // fgExpandRuntimeLookups : partially expand runtime lookups helper calls
 //                          to add a nullcheck [+ size check] and a fast path

src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs

@@ -233,10 +233,6 @@ public unsafe struct CORINFO_RUNTIME_LOOKUP
         public byte _testForNull;
         public bool testForNull { get { return _testForNull != 0; } set { _testForNull = value ? (byte)1 : (byte)0; } }
 
-        // If set, test the lowest bit and dereference if set (see code:FixupPointer)
-        public byte _testForFixup;
-        public bool testForFixup { get { return _testForFixup != 0; } set { _testForFixup = value ? (byte)1 : (byte)0; } }
-
         public ushort sizeOffset;
         public IntPtr offset0;
         public IntPtr offset1;

src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

@@ -289,7 +289,6 @@ private void ComputeLookup(ref CORINFO_RESOLVED_TOKEN pResolvedToken, object ent
                         lookup.runtimeLookup.offset1 = IntPtr.Zero;
                     }
                     lookup.runtimeLookup.sizeOffset = CORINFO.CORINFO_NO_SIZE_CHECK;
-                    lookup.runtimeLookup.testForFixup = false; // TODO: this will be needed in true multifile
                     lookup.runtimeLookup.testForNull = false;
                     lookup.runtimeLookup.indirectFirstOffset = false;
                     lookup.runtimeLookup.indirectSecondOffset = false;

src/coreclr/tools/superpmi/superpmi-shared/agnostic.h

@@ -249,7 +249,6 @@ struct Agnostic_CORINFO_RUNTIME_LOOKUP
     DWORD     helper;
     DWORD     indirections;
     DWORD     testForNull;
-    DWORD     testForFixup;
     WORD      sizeOffset;
     DWORDLONG offsets[CORINFO_MAXINDIRECTIONS];
     DWORD     indirectFirstOffset;

src/coreclr/tools/superpmi/superpmi-shared/spmidumphelper.cpp

@@ -56,8 +56,8 @@ std::string SpmiDumpHelper::DumpAgnostic_CORINFO_RUNTIME_LOOKUP(
     const Agnostic_CORINFO_RUNTIME_LOOKUP& lookup)
 {
     char buffer[MAX_BUFFER_SIZE];
-    sprintf_s(buffer, MAX_BUFFER_SIZE, " sig-%016llX hlp-%u ind-%u tfn-%u tff-%u so-%u { ", lookup.signature, lookup.helper,
-              lookup.indirections, lookup.testForNull, lookup.testForFixup, lookup.sizeOffset);
+    sprintf_s(buffer, MAX_BUFFER_SIZE, " sig-%016llX hlp-%u ind-%u tfn-%u so-%u { ", lookup.signature, lookup.helper,
+              lookup.indirections, lookup.testForNull, lookup.sizeOffset);
     std::string resultDump(buffer);
     for (int i = 0; i < CORINFO_MAXINDIRECTIONS; i++)
     {

src/coreclr/tools/superpmi/superpmi-shared/spmirecordhelper.h

@@ -486,7 +486,6 @@ inline Agnostic_CORINFO_RUNTIME_LOOKUP SpmiRecordsHelper::StoreAgnostic_CORINFO_
     runtimeLookup.helper               = (DWORD)pLookup->helper;
     runtimeLookup.indirections         = (DWORD)pLookup->indirections;
     runtimeLookup.testForNull          = (DWORD)pLookup->testForNull;
-    runtimeLookup.testForFixup         = (DWORD)pLookup->testForFixup;
     runtimeLookup.sizeOffset           = pLookup->sizeOffset;
     runtimeLookup.indirectFirstOffset  = (DWORD)pLookup->indirectFirstOffset;
     runtimeLookup.indirectSecondOffset = (DWORD)pLookup->indirectSecondOffset;
@@ -503,7 +502,6 @@ inline CORINFO_RUNTIME_LOOKUP SpmiRecordsHelper::RestoreCORINFO_RUNTIME_LOOKUP(
     runtimeLookup.helper               = (CorInfoHelpFunc)lookup.helper;
     runtimeLookup.indirections         = (WORD)lookup.indirections;
     runtimeLookup.testForNull          = lookup.testForNull != 0;
-    runtimeLookup.testForFixup         = lookup.testForFixup != 0;
     runtimeLookup.sizeOffset           = lookup.sizeOffset;
     runtimeLookup.indirectFirstOffset  = lookup.indirectFirstOffset != 0;
     runtimeLookup.indirectSecondOffset = lookup.indirectSecondOffset != 0;

src/coreclr/vm/jitinterface.cpp

@@ -2874,7 +2874,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
             {
                 pResult->indirections = 2;
                 pResult->testForNull = 0;
-                pResult->testForFixup = 0;
                 pResult->offsets[0] = offsetof(InstantiatedMethodDesc, m_pPerInstInfo);
 
                 uint32_t data;
@@ -2914,7 +2913,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
             // Just use the method descriptor that was passed in!
             pResult->indirections = 0;
             pResult->testForNull = 0;
-            pResult->testForFixup = 0;
 
             return;
         }
@@ -2951,7 +2949,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
             {
                 pResult->indirections = 3;
                 pResult->testForNull = 0;
-                pResult->testForFixup = 0;
                 pResult->offsets[0] = MethodTable::GetOffsetOfPerInstInfo();
                 pResult->offsets[1] = sizeof(TypeHandle*) * (pContextMT->GetNumDicts() - 1);
                 uint32_t data;
@@ -2974,7 +2971,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
                 // Just use the vtable pointer itself!
                 pResult->indirections = 0;
                 pResult->testForNull = 0;
-                pResult->testForFixup = 0;
 
                 return;
             }
@@ -3166,7 +3162,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
         if (DictionaryLayout::FindToken(pContextMD, pContextMD->GetLoaderAllocator(), 1, &sigBuilder, NULL, signatureSource, pResult, &slot))
         {
             pResult->testForNull = 1;
-            pResult->testForFixup = 0;
             int minDictSize = pContextMD->GetNumGenericMethodArgs() + 1 + pContextMD->GetDictionaryLayout()->GetNumInitialSlots();
             if (slot >= minDictSize)
             {
@@ -3185,7 +3180,6 @@ void CEEInfo::ComputeRuntimeLookupForSharedGenericToken(DictionaryEntryKind entr
         if (DictionaryLayout::FindToken(pContextMT, pContextMT->GetLoaderAllocator(), 2, &sigBuilder, NULL, signatureSource, pResult, &slot))
         {
             pResult->testForNull = 1;
-            pResult->testForFixup = 0;
             int minDictSize = pContextMT->GetNumGenericArgs() + 1 + pContextMT->GetClass()->GetDictionaryLayout()->GetNumInitialSlots();
             if (slot >= minDictSize)
             {

src/coreclr/vm/prestub.cpp

@@ -2843,9 +2843,8 @@ void ProcessDynamicDictionaryLookup(TransitionBlock *           pTransitionBlock
 
     TADDR genericContextPtr = *(TADDR*)GetFirstArgumentRegisterValuePtr(pTransitionBlock);
 
-    pResult->testForFixup = pResult->testForNull = false;
     pResult->signature = NULL;
-
+    pResult->testForNull = false;
     pResult->indirectFirstOffset = 0;
     pResult->indirectSecondOffset = 0;
     // Dictionary size checks skipped by default, unless we decide otherwise