From 755f01659f03196487ec41225de8956911f8049b Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Mon, 21 Sep 2020 11:50:03 -0700 Subject: [PATCH] Annotate handle indirections with GTF_IND_INVARIANT and add validation to fgDebugCheckFlags (#42021) * Rename GTF_ICON_PTR_HDL to GTF_ICON_PTR_GLOBAL - Use this whenever we are embedding a pointer to mutable data from the VM state * Rename GTF_ICON_PSTR_HDL to GTF_ICON_CONST_PTR, use this when the constant is a pointer to immutable data, (e.g. IAT_PPVALUE) * Use GTF_ICON_TOKEN_HDL only for a constant that is a token handle, other than class, method or field. * In gtDispConst - print H or I when we have a handle, or O for a field offset In gtNewStringLiteralNode - Added assert for non-CoreRT case of a IAT_VALUE of a TYP_REF * In optHoistThisLoop - print if the loop has a single exit or multiple exits as this can currently significantly limit our hoisting optimization. * Added assert for later lowering of IAT_PPVALUE in Lowering::LowerDirectCall and comment in Lowering::LowerNonvirtPinvokeCall where we currently allow this late expansion. * Added function fgDebugCheckDispFlags to support extra flag checking of GTF_IND_INVARIANT Disable the GTF_IND_INVARIANT for the cases that currentlt do not set it correctly. * Added additional support for checking of GTF_IND_NONFAULTING in fgDebugCheckFlags (disabled) Minor changes with GTF_IND_NONFAULTING Added assert(!"Check this") Add check for GTF_ICON_PINVKI_HDL // Eventually remove, as this is immutable * Changed a GTF_ICON_FIELD_HDL to be invariant in fgMorphField Added assert(!"Unreached GTF_IND_INVARIANT") in fgMorphField Fixed IAT_PPVALUe and modified IAT_PVALUE cases in fgMorphLeaf for GT_FTN_ADDR * Mark indirection of handles created by gtNewIconEmbHndNode as invariant This was previously #ifdef out ran jit-format * Enable checking of GTF_IND_INVARIANT in fgDebugCheckFlags * Fix Compiler::fgMorphLeaf for extra fgMorpgTree in GT_FTN_ADDR IAT_VALUE case Removed #if 0 code * Reverse the if-the-else block for creating a static field address or value Remove the unreachable block * Added asserts to gtNewIndOfIconHandleNode if isInvariant is set incorrectly, set GTF_GLOB_REF for all class static indirections Check that all class static indirections have GTF_GLOB_REF set Changed two asserts to be noway_asserts Changed the indirection that points to the array initialization blob to be a GTF_ICON_CONST_PTR instead of a GTF_ICON_STATIC_HDL. Changed the indirection that fetches the field offset value for ReadyToRun to be a GTF_ICON_CONST_PTR instead of a GTF_ICON_FIELD_HDL. * ifdef 0 code to touch the codegen of Generic methods via GTF_DONT_CSE * Rename of GTF_ICON_PTR_GLOBAL to GTF_ICON_GLOBAL_PTR * Refactor morph of static field and eliminate the dead code * Add method header for fgDebugCheckDispFlags Rename of GTF_ICON_GLOBAL_PTR) for emitarm64.ccp --- src/coreclr/src/jit/compiler.cpp | 8 +-- src/coreclr/src/jit/compiler.h | 1 + src/coreclr/src/jit/compiler.hpp | 2 +- src/coreclr/src/jit/emitarm64.cpp | 10 +++- src/coreclr/src/jit/flowgraph.cpp | 118 +++++++++++++++++++++++++++++++++---- src/coreclr/src/jit/gentree.cpp | 73 +++++++++++++++++------ src/coreclr/src/jit/gentree.h | 6 +- src/coreclr/src/jit/importer.cpp | 71 +++++++++++----------- src/coreclr/src/jit/lower.cpp | 21 ++++++- src/coreclr/src/jit/morph.cpp | 121 +++++++++++++++++--------------------- src/coreclr/src/jit/optimizer.cpp | 1 + 11 files changed, 288 insertions(+), 144 deletions(-) diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 561b8f7..8e9508b 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -8890,14 +8890,14 @@ void cTreeFlags(Compiler* comp, GenTree* tree) chars += printf("[ICON_STR_HDL]"); break; - case GTF_ICON_PSTR_HDL: + case GTF_ICON_CONST_PTR: - chars += printf("[ICON_PSTR_HDL]"); + chars += printf("[ICON_CONST_PTR]"); break; - case GTF_ICON_PTR_HDL: + case GTF_ICON_GLOBAL_PTR: - chars += printf("[ICON_PTR_HDL]"); + chars += printf("[ICON_GLOBAL_PTR]"); break; case GTF_ICON_VARG_HDL: diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 45033f2..164da5e 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -5295,6 +5295,7 @@ public: void fgDebugCheckNodesUniqueness(); void fgDebugCheckFlags(GenTree* tree); + void fgDebugCheckDispFlags(GenTree* tree, unsigned dispFlags, unsigned debugFlags); void fgDebugCheckFlagsHelper(GenTree* tree, unsigned treeFlags, unsigned chkFlags); void fgDebugCheckTryFinallyExits(); #endif diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index ca3ed3c..3fb8413 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -1148,7 +1148,7 @@ inline GenTreeCall* Compiler::gtNewRuntimeLookupHelperCallNode(CORINFO_RUNTIME_L GenTree* ctxTree, void* compileTimeHandle) { - GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_TOKEN_HDL, compileTimeHandle); + GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_GLOBAL_PTR, compileTimeHandle); GenTreeCall::Use* helperArgs = gtNewCallArgs(ctxTree, argNode); return gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, helperArgs); diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 780f154..85e3d14 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -12309,7 +12309,15 @@ void emitter::emitDispIns( { targetName = "SetGlobalSecurityCookie"; } - else if ((idFlags == GTF_ICON_STR_HDL) || (idFlags == GTF_ICON_PSTR_HDL)) + else if (idFlags == GTF_ICON_CONST_PTR) + { + targetName = "const ptr"; + } + else if (idFlags == GTF_ICON_GLOBAL_PTR) + { + targetName = "global ptr"; + } + else if (idFlags == GTF_ICON_STR_HDL) { stringLiteral = emitComp->eeGetCPString(targetHandle); // Note that eGetCPString isn't currently implemented on Linux/ARM diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index 80821fc..4cb1428 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -4371,7 +4371,7 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block) { // Use a double indirection GenTree* addr = - gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pAddrOfCaptureThreadGlobal, GTF_ICON_PTR_HDL, true); + gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pAddrOfCaptureThreadGlobal, GTF_ICON_CONST_PTR, true); value = gtNewOperNode(GT_IND, TYP_INT, addr); // This indirection won't cause an exception. @@ -4380,7 +4380,7 @@ BasicBlock* Compiler::fgCreateGCPoll(GCPollType pollType, BasicBlock* block) else { // Use a single indirection - value = gtNewIndOfIconHandleNode(TYP_INT, (size_t)addrTrap, GTF_ICON_PTR_HDL, false); + value = gtNewIndOfIconHandleNode(TYP_INT, (size_t)addrTrap, GTF_ICON_GLOBAL_PTR, false); } // Treat the reading of g_TrapReturningThreads as volatile. @@ -9525,7 +9525,8 @@ void Compiler::fgAddInternal() if (dbgHandle || pDbgHandle) { - GenTree* embNode = gtNewIconEmbHndNode(dbgHandle, pDbgHandle, GTF_ICON_TOKEN_HDL, info.compMethodHnd); + // Test the JustMyCode VM global state variable + GenTree* embNode = gtNewIconEmbHndNode(dbgHandle, pDbgHandle, GTF_ICON_GLOBAL_PTR, info.compMethodHnd); GenTree* guardCheckVal = gtNewOperNode(GT_IND, TYP_INT, embNode); GenTree* guardCheckCond = gtNewOperNode(GT_EQ, TYP_INT, guardCheckVal, gtNewZeroConNode(TYP_INT)); @@ -21854,6 +21855,77 @@ void Compiler::fgDebugCheckFlags(GenTree* tree) case GT_ADDR: assert(!op1->CanCSE()); + case GT_IND: + // Do we have a constant integer address as op1? + // + if (op1->OperGet() == GT_CNS_INT) + { + // Is this constant a handle of some kind? + // + unsigned handleKind = (op1->gtFlags & GTF_ICON_HDL_MASK); + if (handleKind != 0) + { + // Is the GTF_IND_INVARIANT flag set or unset? + // + bool invariantFlag = (tree->gtFlags & GTF_IND_INVARIANT) != 0; + if (invariantFlag) + { + // Record the state of the GTF_IND_INVARIANT flags into 'chkFlags' + chkFlags |= GTF_IND_INVARIANT; + } + + // Is the GTF_IND_NONFAULTING flag set or unset? + // + bool nonFaultingFlag = (tree->gtFlags & GTF_IND_NONFAULTING) != 0; + if (nonFaultingFlag) + { + // Record the state of the GTF_IND_NONFAULTING flags into 'chkFlags' + chkFlags |= GTF_IND_NONFAULTING; + } + assert(nonFaultingFlag); // Currently this should always be set for all handle kinds + + // Some of these aren't handles to invariant data... + // + if ((handleKind == GTF_ICON_STATIC_HDL) || // Pointer to a mutable class Static variable + (handleKind == GTF_ICON_BBC_PTR) || // Pointer to a mutable basic block count value + (handleKind == GTF_ICON_GLOBAL_PTR)) // Pointer to mutable data from the VM state + + { + // We expect the Invariant flag to be unset for this handleKind + // If it is set then we will assert with "unexpected GTF_IND_INVARIANT flag set ... + // + if (handleKind == GTF_ICON_STATIC_HDL) + { + // We expect the GTF_GLOB_REF flag to be set for this handleKind + // If it is not set then we will assert with "Missing flags on tree" + // + treeFlags |= GTF_GLOB_REF; + } + } + else // All the other handle indirections are considered invariant + { + // We expect the Invariant flag to be set for this handleKind + // If it is not set then we will assert with "Missing flags on tree" + // + treeFlags |= GTF_IND_INVARIANT; + } + + // We currently expect all handle kinds to be nonFaulting + // + treeFlags |= GTF_IND_NONFAULTING; + + // Matrix for GTF_IND_INVARIANT (treeFlags and chkFlags) + // + // chkFlags INVARIANT value + // 0 1 + // +--------------+----------------+ + // treeFlags 0 | OK | Missing Flag | + // INVARIANT +--------------+----------------+ + // value: 1 | Extra Flag | OK | + // +--------------+----------------+ + } + } + default: break; } @@ -22103,6 +22175,27 @@ void Compiler::fgDebugCheckFlags(GenTree* tree) } //------------------------------------------------------------------------------ +// fgDebugCheckDispFlags: +// Wrapper function that displays two GTF_IND_ flags +// and then calls ftDispFlags to display the rest. +// +// Arguments: +// tree - Tree whose flags are being checked +// dispFlags - the first argument for gtDispFlags +// ands hold GTF_IND_INVARIANT and GTF_IND_NONFLUALTING +// debugFlags - the second argument to gtDispFlags +// +void Compiler::fgDebugCheckDispFlags(GenTree* tree, unsigned dispFlags, unsigned debugFlags) +{ + if (tree->OperGet() == GT_IND) + { + printf("%c", (dispFlags & GTF_IND_INVARIANT) ? '#' : '-'); + printf("%c", (dispFlags & GTF_IND_NONFAULTING) ? 'n' : '-'); + } + GenTree::gtDispFlags(dispFlags, debugFlags); +} + +//------------------------------------------------------------------------------ // fgDebugCheckFlagsHelper : Check if all bits that are set in chkFlags are also set in treeFlags. // // @@ -22120,7 +22213,7 @@ void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, unsigned treeFlags, unsign { // Print the tree so we can see it in the log. printf("Missing flags on tree [%06d]: ", dspTreeID(tree)); - GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE); + Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE); printf("\n"); gtDispTree(tree); @@ -22128,26 +22221,29 @@ void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, unsigned treeFlags, unsign // Print the tree again so we can see it right after we hook up the debugger. printf("Missing flags on tree [%06d]: ", dspTreeID(tree)); - GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE); + Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE); printf("\n"); gtDispTree(tree); } else if (treeFlags & ~chkFlags) { - // TODO: We are currently only checking extra GTF_EXCEPT, GTF_ASG, and GTF_CALL flags. - if ((treeFlags & ~chkFlags & ~GTF_GLOB_REF & ~GTF_ORDER_SIDEEFF) != 0) + // We can't/don't consider these flags (GTF_GLOB_REF or GTF_ORDER_SIDEEFF) as being "extra" flags + // + unsigned flagsToCheck = ~GTF_GLOB_REF & ~GTF_ORDER_SIDEEFF; + + if ((treeFlags & ~chkFlags & flagsToCheck) != 0) { // Print the tree so we can see it in the log. - printf("Extra flags on parent tree [%X]: ", tree); - GenTree::gtDispFlags(treeFlags & ~chkFlags, GTF_DEBUG_NONE); + printf("Extra flags on tree [%06d]: ", dspTreeID(tree)); + Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE); printf("\n"); gtDispTree(tree); noway_assert(!"Extra flags on tree"); // Print the tree again so we can see it right after we hook up the debugger. - printf("Extra flags on parent tree [%X]: ", tree); - GenTree::gtDispFlags(treeFlags & ~chkFlags, GTF_DEBUG_NONE); + printf("Extra flags on tree [%06d]: ", dspTreeID(tree)); + Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE); printf("\n"); gtDispTree(tree); } diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index a0fa70f..12e61e8 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -5987,18 +5987,25 @@ GenTree* Compiler::gtNewIndOfIconHandleNode(var_types indType, size_t addr, unsi // indNode->gtFlags |= GTF_IND_NONFAULTING; - // String Literal handles are indirections that return a TYP_REF. - // They are pointers into the GC heap and they are not invariant - // as the address is a reportable GC-root and as such it can be - // modified during a GC collection + // String Literal handles are indirections that return a TYP_REF, and + // these are pointers into the GC heap. We don't currently have any + // TYP_BYREF pointers, but if we did they also must be pointers into the GC heap. // - if (indType == TYP_REF) + // Also every GTF_ICON_STATIC_HDL also must be a pointer into the GC heap + // we will set GTF_GLOB_REF for these kinds of references. + // + if ((varTypeIsGC(indType)) || (iconFlags == GTF_ICON_STATIC_HDL)) { - // This indirection points into the gloabal heap + // This indirection also points into the gloabal heap indNode->gtFlags |= GTF_GLOB_REF; } + if (isInvariant) { + assert(iconFlags != GTF_ICON_STATIC_HDL); // Pointer to a mutable class Static variable + assert(iconFlags != GTF_ICON_BBC_PTR); // Pointer to a mutable basic block count value + assert(iconFlags != GTF_ICON_GLOBAL_PTR); // Pointer to mutable data from the VM state + // This indirection also is invariant. indNode->gtFlags |= GTF_IND_INVARIANT; } @@ -6044,12 +6051,9 @@ GenTree* Compiler::gtNewIconEmbHndNode(void* value, void* pValue, unsigned iconF // This indirection won't cause an exception. handleNode->gtFlags |= GTF_IND_NONFAULTING; -#if 0 - // It should also be invariant, but marking it as such leads to bad diffs. // This indirection also is invariant. handleNode->gtFlags |= GTF_IND_INVARIANT; -#endif } iconNode->AsIntCon()->gtCompileTimeHandle = (size_t)compileTimeHandle; @@ -6064,7 +6068,14 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue) switch (iat) { - case IAT_VALUE: // constructStringLiteral in CoreRT case can return IAT_VALUE + case IAT_VALUE: + // For CoreRT only - Constant object can be a frozen string. + if (!IsTargetAbi(CORINFO_CORERT_ABI)) + { + // Non CoreRT - This case is illegal, creating a TYP_REF from an INT_CNS + noway_assert(!"unreachable IAT_VALUE case in gtNewStringLiteralNode"); + } + tree = gtNewIconEmbHndNode(pValue, nullptr, GTF_ICON_STR_HDL, nullptr); tree->gtType = TYP_REF; #ifdef DEBUG @@ -6083,7 +6094,7 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue) case IAT_PPVALUE: // The value needs to be accessed via a double indirection // Create the first indirection - tree = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pValue, GTF_ICON_PSTR_HDL, true); + tree = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pValue, GTF_ICON_CONST_PTR, true); #ifdef DEBUG tree->gtGetOp1()->AsIntCon()->gtTargetHandle = (size_t)pValue; #endif @@ -10303,6 +10314,31 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _ printf((tree->gtFlags & GTF_JCMP_EQ) ? "EQ" : "NE"); goto DASH; + case GT_CNS_INT: + if (tree->IsIconHandle()) + { + if ((tree->gtFlags & GTF_ICON_INITCLASS) != 0) + { + printf("I"); // Static Field handle with INITCLASS requirement + --msgLength; + break; + } + else if ((tree->gtFlags & GTF_ICON_FIELD_OFF) != 0) + { + printf("O"); + --msgLength; + break; + } + else + { + // Some other handle + printf("H"); + --msgLength; + break; + } + } + goto DASH; + default: DASH: printf("-"); @@ -10868,13 +10904,14 @@ void Compiler::gtDispConst(GenTree* tree) if (tree->IsIconHandle(GTF_ICON_STR_HDL)) { const WCHAR* str = eeGetCPString(tree->AsIntCon()->gtIconVal); - if (str != nullptr) + // If *str points to a '\0' then don't print the string's values + if ((str != nullptr) && (*str != '\0')) { printf(" 0x%X \"%S\"", dspPtr(tree->AsIntCon()->gtIconVal), str); } - else + else // We can't print the value of the string { - // Note that eGetCPString isn't currently implemented on Linux/ARM + // Note that eeGetCPString isn't currently implemented on Linux/ARM // and instead always returns nullptr printf(" 0x%X [ICON_STR_HDL]", dspPtr(tree->AsIntCon()->gtIconVal)); } @@ -10940,11 +10977,11 @@ void Compiler::gtDispConst(GenTree* tree) case GTF_ICON_STR_HDL: unreached(); // This case is handled above break; - case GTF_ICON_PSTR_HDL: - printf(" pstr"); + case GTF_ICON_CONST_PTR: + printf(" const ptr"); break; - case GTF_ICON_PTR_HDL: - printf(" ptr"); + case GTF_ICON_GLOBAL_PTR: + printf(" global ptr"); break; case GTF_ICON_VARG_HDL: printf(" vararg"); diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 934a141..a369a79 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -904,11 +904,11 @@ public: #define GTF_ICON_FIELD_HDL 0x40000000 // GT_CNS_INT -- constant is a field handle #define GTF_ICON_STATIC_HDL 0x50000000 // GT_CNS_INT -- constant is a handle to static data #define GTF_ICON_STR_HDL 0x60000000 // GT_CNS_INT -- constant is a string handle -#define GTF_ICON_PSTR_HDL 0x70000000 // GT_CNS_INT -- constant is a ptr to a string handle -#define GTF_ICON_PTR_HDL 0x80000000 // GT_CNS_INT -- constant is a ldptr handle +#define GTF_ICON_CONST_PTR 0x70000000 // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE) +#define GTF_ICON_GLOBAL_PTR 0x80000000 // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state) #define GTF_ICON_VARG_HDL 0x90000000 // GT_CNS_INT -- constant is a var arg cookie handle #define GTF_ICON_PINVKI_HDL 0xA0000000 // GT_CNS_INT -- constant is a pinvoke calli handle -#define GTF_ICON_TOKEN_HDL 0xB0000000 // GT_CNS_INT -- constant is a token handle +#define GTF_ICON_TOKEN_HDL 0xB0000000 // GT_CNS_INT -- constant is a token handle (other than class, method or field) #define GTF_ICON_TLS_HDL 0xC0000000 // GT_CNS_INT -- constant is a TLS ref with offset #define GTF_ICON_FTN_ADDR 0xD0000000 // GT_CNS_INT -- constant is a function address #define GTF_ICON_CIDMID_HDL 0xE0000000 // GT_CNS_INT -- constant is a class ID or a module ID diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 0b342ef..d5909f9 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -2081,7 +2081,9 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken void* compileTimeHandle) { GenTree* ctxTree = getRuntimeContextTree(pLookup->lookupKind.runtimeLookupKind); - +#if 0 + ctxTree->gtFlags |= GTF_DONT_CSE; // ToDo Remove this +#endif CORINFO_RUNTIME_LOOKUP* pRuntimeLookup = &pLookup->runtimeLookup; // It's available only via the run-time helper function if (pRuntimeLookup->indirections == CORINFO_USEHELPER) @@ -2201,8 +2203,9 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken GenTree* handleForNullCheck = gtNewOperNode(GT_IND, TYP_I_IMPL, slotPtrTree); handleForNullCheck->gtFlags |= GTF_IND_NONFAULTING; - // Call to helper - GenTree* argNode = gtNewIconEmbHndNode(pRuntimeLookup->signature, nullptr, GTF_ICON_TOKEN_HDL, 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::Use* helperArgs = gtNewCallArgs(ctxTree, argNode); GenTreeCall* helperCall = gtNewHelperCallNode(pRuntimeLookup->helper, TYP_I_IMPL, helperArgs); @@ -3440,7 +3443,8 @@ GenTree* Compiler::impInitializeArrayIntrinsic(CORINFO_SIG_INFO* sig) GenTree* dstAddr = gtNewOperNode(GT_ADD, TYP_BYREF, arrayLocalNode, gtNewIconNode(dataOffset, TYP_I_IMPL)); GenTree* dst = new (this, GT_BLK) GenTreeBlk(GT_BLK, TYP_STRUCT, dstAddr, typGetBlkLayout(blkSize)); - GenTree* src = gtNewIndOfIconHandleNode(TYP_STRUCT, (size_t)initData, GTF_ICON_STATIC_HDL, false); + GenTree* src = gtNewIndOfIconHandleNode(TYP_STRUCT, (size_t)initData, GTF_ICON_CONST_PTR, true); + #ifdef DEBUG src->gtGetOp1()->AsIntCon()->gtTargetHandle = THT_IntializeArrayIntrinsics; #endif @@ -7291,7 +7295,32 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT default: { - if (!(access & CORINFO_ACCESS_ADDRESS)) + // Do we need the addrees of a static field? + // + if (access & CORINFO_ACCESS_ADDRESS) + { + void** pFldAddr = nullptr; + void* fldAddr = info.compCompHnd->getFieldAddress(pResolvedToken->hField, (void**)&pFldAddr); + + // We should always be able to access this static's address directly + // + assert(pFldAddr == nullptr); + + FieldSeqNode* fldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField); + + /* Create the data member node */ + op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL, + fldSeq); +#ifdef DEBUG + op1->AsIntCon()->gtTargetHandle = op1->AsIntCon()->gtIconVal; +#endif + + if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS) + { + op1->gtFlags |= GTF_ICON_INITCLASS; + } + } + else // We need the value of a static field { // In future, it may be better to just create the right tree here instead of folding it later. op1 = gtNewFieldRef(lclTyp, pResolvedToken->hField); @@ -7324,38 +7353,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT return op1; } - else - { - void** pFldAddr = nullptr; - void* fldAddr = info.compCompHnd->getFieldAddress(pResolvedToken->hField, (void**)&pFldAddr); - - FieldSeqNode* fldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField); - /* Create the data member node */ - op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL, - fldSeq); -#ifdef DEBUG - op1->AsIntCon()->gtTargetHandle = op1->AsIntCon()->gtIconVal; -#endif - - if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS) - { - op1->gtFlags |= GTF_ICON_INITCLASS; - } - - if (pFldAddr != nullptr) - { - // There are two cases here, either the static is RVA based, - // in which case the type of the FIELD node is not a GC type - // and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is - // a GC type and the handle to it is a TYP_BYREF in the GC heap - // because handles to statics now go into the large object heap - - var_types handleTyp = (var_types)(varTypeIsGC(lclTyp) ? TYP_BYREF : TYP_I_IMPL); - op1 = gtNewOperNode(GT_IND, handleTyp, op1); - op1->gtFlags |= GTF_IND_INVARIANT | GTF_IND_NONFAULTING; - } - } break; } } @@ -14582,6 +14580,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) info.compCompHnd->getFieldAddress(resolvedToken.hField, (void**)&pFldAddr); // We should always be able to access this static's address directly + // assert(pFldAddr == nullptr); op1 = impImportStaticReadOnlyField(fldAddr, lclTyp); diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index d047d00..2cd9022 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -3691,10 +3691,17 @@ GenTree* Lowering::LowerDirectCall(GenTreeCall* call) // Non-virtual direct calls to addresses accessed by // a double indirection. // - // Double-indirection. Load the address into a register - // and call indirectly through the register + + // Expanding an IAT_PPVALUE here, will lose the opportunity + // to Hoist/CSE the first indirection as it is an invariant load + // + assert(!"IAT_PPVALUE case in LowerDirectCall"); + noway_assert(helperNum == CORINFO_HELP_UNDEF); result = AddrGen(addr); + // Double-indirection. Load the address into a register + // and call indirectly through the register + // result = Ind(Ind(result)); break; @@ -4488,10 +4495,20 @@ GenTree* Lowering::LowerNonvirtPinvokeCall(GenTreeCall* call) break; case IAT_PPVALUE: + // ToDo: Expanding an IAT_PPVALUE here, loses the opportunity + // to Hoist/CSE the first indirection as it is an invariant load + // + // This case currently occurs today when we make PInvoke calls in crossgen + // + // assert(!"IAT_PPVALUE in Lowering::LowerNonvirtPinvokeCall"); + addrTree = AddrGen(addr); #ifdef DEBUG addrTree->AsIntCon()->gtTargetHandle = (size_t)methHnd; #endif + // Double-indirection. Load the address into a register + // and call indirectly through the register + // result = Ind(Ind(addrTree)); break; diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index fdb3942..6b75dad 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -6126,7 +6126,7 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) if (tree->AsField()->gtFieldLookup.accessType == IAT_PVALUE) { offsetNode = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)tree->AsField()->gtFieldLookup.addr, - GTF_ICON_FIELD_HDL, false); + GTF_ICON_CONST_PTR, true); #ifdef DEBUG offsetNode->gtGetOp1()->AsIntCon()->gtTargetHandle = (size_t)symHnd; #endif @@ -6228,7 +6228,7 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } else { - dllRef = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pIdAddr, GTF_ICON_STATIC_HDL, true); + dllRef = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)pIdAddr, GTF_ICON_CONST_PTR, true); // Next we multiply by 4 dllRef = gtNewOperNode(GT_MUL, TYP_I_IMPL, dllRef, gtNewIconNode(4, TYP_I_IMPL)); @@ -6288,51 +6288,21 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) void** pFldAddr = nullptr; void* fldAddr = info.compCompHnd->getFieldAddress(symHnd, (void**)&pFldAddr); - if (pFldAddr == nullptr) - { -#ifdef TARGET_64BIT - if (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr)) - { - // The address is not directly addressible, so force it into a - // constant, so we handle it properly - - GenTree* addr = gtNewIconHandleNode((size_t)fldAddr, GTF_ICON_STATIC_HDL); - addr->gtType = TYP_I_IMPL; - FieldSeqNode* fieldSeq = - fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); - addr->AsIntCon()->gtFieldSeq = fieldSeq; - // Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS - if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0) - { - tree->gtFlags &= ~GTF_FLD_INITCLASS; - addr->gtFlags |= GTF_ICON_INITCLASS; - } - - tree->SetOper(GT_IND); - tree->AsOp()->gtOp1 = addr; - - return fgMorphSmpOp(tree); - } - else -#endif // TARGET_64BIT - { - // Only volatile or classinit could be set, and they map over - noway_assert((tree->gtFlags & ~(GTF_FLD_VOLATILE | GTF_FLD_INITCLASS | GTF_COMMON_MASK)) == 0); - static_assert_no_msg(GTF_FLD_VOLATILE == GTF_CLS_VAR_VOLATILE); - static_assert_no_msg(GTF_FLD_INITCLASS == GTF_CLS_VAR_INITCLASS); - tree->SetOper(GT_CLS_VAR); - tree->AsClsVar()->gtClsVarHnd = symHnd; - FieldSeqNode* fieldSeq = - fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); - tree->AsClsVar()->gtFieldSeq = fieldSeq; - } + // We should always be able to access this static field address directly + // + assert(pFldAddr == nullptr); - return tree; - } - else +#ifdef TARGET_64BIT + if (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr)) { - GenTree* addr = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL); + // The address is not directly addressible, so force it into a + // constant, so we handle it properly + GenTree* addr = gtNewIconHandleNode((size_t)fldAddr, GTF_ICON_STATIC_HDL); + addr->gtType = TYP_I_IMPL; + FieldSeqNode* fieldSeq = + fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); + addr->AsIntCon()->gtFieldSeq = fieldSeq; // Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0) { @@ -6340,19 +6310,26 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) addr->gtFlags |= GTF_ICON_INITCLASS; } - // There are two cases here, either the static is RVA based, - // in which case the type of the FIELD node is not a GC type - // and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is - // a GC type and the handle to it is a TYP_BYREF in the GC heap - // because handles to statics now go into the large object heap - - var_types handleTyp = (var_types)(varTypeIsGC(tree->TypeGet()) ? TYP_BYREF : TYP_I_IMPL); - GenTree* op1 = gtNewOperNode(GT_IND, handleTyp, addr); - op1->gtFlags |= GTF_IND_INVARIANT; - tree->SetOper(GT_IND); - tree->AsOp()->gtOp1 = op1; + tree->AsOp()->gtOp1 = addr; + + return fgMorphSmpOp(tree); + } + else +#endif // TARGET_64BIT + { + // Only volatile or classinit could be set, and they map over + noway_assert((tree->gtFlags & ~(GTF_FLD_VOLATILE | GTF_FLD_INITCLASS | GTF_COMMON_MASK)) == 0); + static_assert_no_msg(GTF_FLD_VOLATILE == GTF_CLS_VAR_VOLATILE); + static_assert_no_msg(GTF_FLD_INITCLASS == GTF_CLS_VAR_INITCLASS); + tree->SetOper(GT_CLS_VAR); + tree->AsClsVar()->gtClsVarHnd = symHnd; + FieldSeqNode* fieldSeq = + fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd); + tree->AsClsVar()->gtFieldSeq = fieldSeq; } + + return tree; } } noway_assert(tree->gtOper == GT_IND); @@ -9265,33 +9242,41 @@ GenTree* Compiler::fgMorphLeaf(GenTree* tree) info.compCompHnd->getFunctionFixedEntryPoint(tree->AsFptrVal()->gtFptrMethod, &addrInfo); } - // Refer to gtNewIconHandleNode() as the template for constructing a constant handle - // - tree->SetOper(GT_CNS_INT); - tree->AsIntConCommon()->SetIconValue(ssize_t(addrInfo.handle)); - tree->gtFlags |= GTF_ICON_FTN_ADDR; - + GenTree* indNode = nullptr; switch (addrInfo.accessType) { case IAT_PPVALUE: - tree = gtNewOperNode(GT_IND, TYP_I_IMPL, tree); - tree->gtFlags |= GTF_IND_INVARIANT; - - __fallthrough; + indNode = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)addrInfo.handle, GTF_ICON_CONST_PTR, true); + + // Add the second indirection + indNode = gtNewOperNode(GT_IND, TYP_I_IMPL, indNode); + // This indirection won't cause an exception. + indNode->gtFlags |= GTF_IND_NONFAULTING; + // This indirection also is invariant. + indNode->gtFlags |= GTF_IND_INVARIANT; + break; case IAT_PVALUE: - tree = gtNewOperNode(GT_IND, TYP_I_IMPL, tree); + indNode = gtNewIndOfIconHandleNode(TYP_I_IMPL, (size_t)addrInfo.handle, GTF_ICON_FTN_ADDR, true); break; case IAT_VALUE: - tree = gtNewOperNode(GT_NOP, tree->TypeGet(), tree); // prevents constant folding + // Refer to gtNewIconHandleNode() as the template for constructing a constant handle + // + tree->SetOper(GT_CNS_INT); + tree->AsIntConCommon()->SetIconValue(ssize_t(addrInfo.handle)); + tree->gtFlags |= GTF_ICON_FTN_ADDR; break; default: noway_assert(!"Unknown addrInfo.accessType"); } - return fgMorphTree(tree); + if (indNode != nullptr) + { + DEBUG_DESTROY_NODE(tree); + tree = fgMorphTree(indNode); + } } return tree; diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index ed5bd11..e7abb09 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -6549,6 +6549,7 @@ void Compiler::optHoistThisLoop(unsigned lnum, LoopHoistContext* hoistCtxt) { printf("optHoistLoopCode for loop L%02u <" FMT_BB ".." FMT_BB ">:\n", lnum, begn, endn); printf(" Loop body %s a call\n", pLoopDsc->lpContainsCall ? "contains" : "does not contain"); + printf(" Loop has %s\n", (pLoopDsc->lpFlags & LPFLG_ONE_EXIT) ? "single exit" : "multiple exits"); } #endif -- 2.7.4