From dba366de7768667ed89d9ea94a8ba25258bd042c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 19 Sep 2023 19:59:11 +0200 Subject: [PATCH 1/6] JIT: Make a quirk in block morphing more explicit Block morphing would create oddly typed trees (mixing up TYP_BYREF/TYP_I_IMPL, e.g. creating LCL_VAR for a TYP_BYREF typed local). The only effect of this was that it would avoid some constant propagation. Make this more explicit by setting GTF_DONT_CSE instead. --- src/coreclr/jit/morphblock.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index cb1fe5106d31ef..347845e7370944 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -1183,8 +1183,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() addrSpillTemp = m_comp->lvaGrabTemp(true DEBUGARG("BlockOp address local")); LclVarDsc* addrSpillDsc = m_comp->lvaGetDesc(addrSpillTemp); - addrSpillDsc->lvType = addrSpill->TypeIs(TYP_REF) ? TYP_REF : TYP_BYREF; // TODO-ASG: zero-diff quirk, delete. - addrSpillStore = m_comp->gtNewTempStore(addrSpillTemp, addrSpill); + addrSpillStore = m_comp->gtNewTempStore(addrSpillTemp, addrSpill); } auto grabAddr = [=, &result](unsigned offs) { @@ -1227,7 +1226,12 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // handling. GenTreeIntCon* fldOffsetNode = m_comp->gtNewIconNode(fullOffs, TYP_I_IMPL); fldOffsetNode->gtFieldSeq = addrBaseOffsFldSeq; - addrClone = m_comp->gtNewOperNode(GT_ADD, TYP_BYREF, addrClone, fldOffsetNode); + addrClone = m_comp->gtNewOperNode(GT_ADD, varTypeIsGC(addrClone) ? TYP_BYREF : TYP_I_IMPL, addrClone, + fldOffsetNode); + // Avoid constant prop propagating each field access with a large + // constant address. TODO-Cleanup: We should tune constant prop to + // have better heuristics around this. + addrClone->gtFlags |= GTF_DONT_CSE; } return addrClone; From d3c831c6865e842f0f9a23e54030b34a7e5b1ed5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 19 Sep 2023 22:38:05 +0200 Subject: [PATCH 2/6] JIT: Reimport full spill clique for I_IMPL<->BYREF mismatches When the JIT sees code that pushes an untracked pointer from one branch, and a tracked pointer from another, it will reimport successors that were already imported under the wrong assumption of an untracked pointer. However, like in the other cases of spill clique handling, it should reimport the entire spill clique, including predecssors and other successors of those; otherwise we can still end up with mistyped LCL_VAR nodes. --- src/coreclr/jit/importer.cpp | 18 ++++++++---------- src/coreclr/jit/lclvars.cpp | 1 - 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 995068c10aef62..0b20f9302915b1 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11387,19 +11387,17 @@ void Compiler::impImportBlock(BasicBlock* block) { GenTree* tree = verCurrentState.esStack[level].val; - /* VC generates code where it pushes a byref from one branch, and an int (ldc.i4 0) from - the other. This should merge to a byref in unverifiable code. - However, if the branch which leaves the TYP_I_IMPL on the stack is imported first, the - successor would be imported assuming there was a TYP_I_IMPL on - the stack. Thus the value would not get GC-tracked. Hence, - change the temp to TYP_BYREF and reimport the successors. - Note: We should only allow this in unverifiable code. - */ + // VC generates code where it pushes a byref from one branch, and an int (ldc.i4 0) from + // the other. This should merge to a byref in unverifiable code. + // However, if the branch which leaves the TYP_I_IMPL on the stack is imported first, the + // successor would be imported assuming there was a TYP_I_IMPL on + // the stack. Thus the value would not get GC-tracked. Hence, + // change the temp to TYP_BYREF and reimport the clique. + // Note: We should only allow this in unverifiable code. if (tree->gtType == TYP_BYREF && lvaTable[tempNum].lvType == TYP_I_IMPL) { lvaTable[tempNum].lvType = TYP_BYREF; - impReimportMarkSuccessors(block); - markImport = true; + reimportSpillClique = true; } #ifdef TARGET_64BIT diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index e1a23da69d4a56..57b4f164fd444c 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -4146,7 +4146,6 @@ void Compiler::lvaMarkLclRefs(GenTree* tree, BasicBlock* block, Statement* stmt, // Check that the LCL_VAR node has the same type as the underlying variable, save a few mismatches we allow. assert(tree->TypeIs(varDsc->TypeGet(), genActualType(varDsc)) || - (tree->TypeIs(TYP_I_IMPL) && (varDsc->TypeGet() == TYP_BYREF)) || // Created for spill clique import. (tree->TypeIs(TYP_BYREF) && (varDsc->TypeGet() == TYP_I_IMPL)) || // Created by inliner substitution. (tree->TypeIs(TYP_INT) && (varDsc->TypeGet() == TYP_LONG))); // Created by "optNarrowTree". } From e4dd78aa8810096eb24326aac32800382e6015c2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 19 Sep 2023 22:56:50 +0200 Subject: [PATCH 3/6] Address feedback, refactor stylistically --- src/coreclr/jit/importer.cpp | 41 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 0b20f9302915b1..42ed1085168d7e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11379,11 +11379,10 @@ void Compiler::impImportBlock(BasicBlock* block) baseTmp = impGetSpillTmpBase(block); } - /* Spill all stack entries into temps */ - unsigned level, tempNum; + // Spill all stack entries into temps JITDUMP("\nSpilling stack entries into temps\n"); - for (level = 0, tempNum = baseTmp; level < verCurrentState.esStackDepth; level++, tempNum++) + for (unsigned level = 0, tempNum = baseTmp; level < verCurrentState.esStackDepth; level++, tempNum++) { GenTree* tree = verCurrentState.esStack[level].val; @@ -11393,22 +11392,22 @@ void Compiler::impImportBlock(BasicBlock* block) // successor would be imported assuming there was a TYP_I_IMPL on // the stack. Thus the value would not get GC-tracked. Hence, // change the temp to TYP_BYREF and reimport the clique. - // Note: We should only allow this in unverifiable code. - if (tree->gtType == TYP_BYREF && lvaTable[tempNum].lvType == TYP_I_IMPL) + LclVarDsc* tempDsc = lvaGetDesc(tempNum); + if (tree->TypeIs(TYP_BYREF) && (tempDsc->TypeGet() == TYP_I_IMPL)) { - lvaTable[tempNum].lvType = TYP_BYREF; - reimportSpillClique = true; + tempDsc->lvType = TYP_BYREF; + reimportSpillClique = true; } #ifdef TARGET_64BIT - if (genActualType(tree->gtType) == TYP_I_IMPL && lvaTable[tempNum].lvType == TYP_INT) + if ((genActualType(tree) == TYP_I_IMPL) && (tempDsc->TypeGet() == TYP_INT)) { // Some other block in the spill clique set this to "int", but now we have "native int". // Change the type and go back to re-import any blocks that used the wrong type. - lvaTable[tempNum].lvType = TYP_I_IMPL; - reimportSpillClique = true; + tempDsc->lvType = TYP_I_IMPL; + reimportSpillClique = true; } - else if (genActualType(tree->gtType) == TYP_INT && lvaTable[tempNum].lvType == TYP_I_IMPL) + else if ((genActualType(tree) == TYP_INT) && (tempDsc->TypeGet() == TYP_I_IMPL)) { // Spill clique has decided this should be "native int", but this block only pushes an "int". // Insert a sign-extension to "native int" so we match the clique. @@ -11423,14 +11422,14 @@ void Compiler::impImportBlock(BasicBlock* block) // imported already, we need to change the type of the local and reimport the spill clique. // If the 'byref' side has imported, we insert a cast from int to 'native int' to match // the 'byref' size. - if (genActualType(tree->gtType) == TYP_BYREF && lvaTable[tempNum].lvType == TYP_INT) + if ((genActualType(tree) == TYP_BYREF) && (tempDsc->TypeGet() == TYP_INT)) { // Some other block in the spill clique set this to "int", but now we have "byref". // Change the type and go back to re-import any blocks that used the wrong type. - lvaTable[tempNum].lvType = TYP_BYREF; - reimportSpillClique = true; + tempDsc->lvType = TYP_BYREF; + reimportSpillClique = true; } - else if (genActualType(tree->gtType) == TYP_INT && lvaTable[tempNum].lvType == TYP_BYREF) + else if ((genActualType(tree) == TYP_INT) && (tempDsc->TypeGet() == TYP_BYREF) { // Spill clique has decided this should be "byref", but this block only pushes an "int". // Insert a sign-extension to "native int" so we match the clique size. @@ -11439,14 +11438,14 @@ void Compiler::impImportBlock(BasicBlock* block) #endif // TARGET_64BIT - if (tree->gtType == TYP_DOUBLE && lvaTable[tempNum].lvType == TYP_FLOAT) + if (tree->TypeIs(TYP_DOUBLE) && (tempDsc->lvType == TYP_FLOAT)) { // Some other block in the spill clique set this to "float", but now we have "double". // Change the type and go back to re-import any blocks that used the wrong type. - lvaTable[tempNum].lvType = TYP_DOUBLE; - reimportSpillClique = true; + tempDsc->lvType = TYP_DOUBLE; + reimportSpillClique = true; } - else if (tree->gtType == TYP_FLOAT && lvaTable[tempNum].lvType == TYP_DOUBLE) + else if (tree->TypeIs(TYP_FLOAT) && (tempDsc->TypeGet() == TYP_DOUBLE)) { // Spill clique has decided this should be "double", but this block only pushes a "float". // Insert a cast to "double" so we match the clique. @@ -11457,7 +11456,7 @@ void Compiler::impImportBlock(BasicBlock* block) are spilling to the temps already used by a previous block), we need to spill addStmt */ - if (addStmt != nullptr && !newTemps && gtHasRef(addStmt->GetRootNode(), tempNum)) + if ((addStmt != nullptr) && !newTemps && gtHasRef(addStmt->GetRootNode(), tempNum)) { GenTree* addTree = addStmt->GetRootNode(); @@ -11486,7 +11485,7 @@ void Compiler::impImportBlock(BasicBlock* block) } else { - assert(addTree->gtOper == GT_SWITCH && genActualTypeIsIntOrI(addTree->AsOp()->gtOp1->TypeGet())); + assert((addTree->gtOper == GT_SWITCH) && genActualTypeIsIntOrI(addTree->AsOp()->gtOp1->TypeGet())); unsigned temp = lvaGrabTemp(true DEBUGARG("spill addStmt SWITCH")); impStoreTemp(temp, addTree->AsOp()->gtOp1, level); From 49f5d6bac7d85109b3b2a400095be3b3d9c0cd83 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 19 Sep 2023 22:58:25 +0200 Subject: [PATCH 4/6] A bit more --- src/coreclr/jit/importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 42ed1085168d7e..8bf972b907b293 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11460,7 +11460,7 @@ void Compiler::impImportBlock(BasicBlock* block) { GenTree* addTree = addStmt->GetRootNode(); - if (addTree->gtOper == GT_JTRUE) + if (addTree->OperIs(GT_JTRUE)) { GenTree* relOp = addTree->AsOp()->gtOp1; assert(relOp->OperIsCompare()); @@ -11485,7 +11485,7 @@ void Compiler::impImportBlock(BasicBlock* block) } else { - assert((addTree->gtOper == GT_SWITCH) && genActualTypeIsIntOrI(addTree->AsOp()->gtOp1->TypeGet())); + assert(addTree->OperIs(GT_SWITCH) && genActualTypeIsIntOrI(addTree->AsOp()->gtOp1->TypeGet())); unsigned temp = lvaGrabTemp(true DEBUGARG("spill addStmt SWITCH")); impStoreTemp(temp, addTree->AsOp()->gtOp1, level); From f6a36684d8100f8c3b76f85f6af13575d0041a41 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 19 Sep 2023 23:17:12 +0200 Subject: [PATCH 5/6] Remove impReimportMarkSuccessors --- src/coreclr/jit/compiler.h | 1 - src/coreclr/jit/importer.cpp | 14 -------------- 2 files changed, 15 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 7d407d9643b41e..d34a9b827a29d2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4239,7 +4239,6 @@ class Compiler void impImportBlockCode(BasicBlock* block); void impReimportMarkBlock(BasicBlock* block); - void impReimportMarkSuccessors(BasicBlock* block); void impVerifyEHBlock(BasicBlock* block, bool isTryStart); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8bf972b907b293..9a27fabbcaa11e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11043,20 +11043,6 @@ inline void Compiler::impReimportMarkBlock(BasicBlock* block) block->bbFlags &= ~BBF_IMPORTED; } -/***************************************************************************** - * Mark the successors of the given block as unimported. - * Note that the caller is responsible for calling impImportBlockPending() - * for all the successors, with the appropriate stack-state. - */ - -void Compiler::impReimportMarkSuccessors(BasicBlock* block) -{ - for (BasicBlock* const succBlock : block->Succs()) - { - impReimportMarkBlock(succBlock); - } -} - /***************************************************************************** * * Filter wrapper to handle only passed in exception code From 5d4c120167ec408f391b5a79478aa9f459f53faf Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 20 Sep 2023 00:39:08 +0200 Subject: [PATCH 6/6] Fix build --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 9a27fabbcaa11e..8d8135d239ecb2 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11415,7 +11415,7 @@ void Compiler::impImportBlock(BasicBlock* block) tempDsc->lvType = TYP_BYREF; reimportSpillClique = true; } - else if ((genActualType(tree) == TYP_INT) && (tempDsc->TypeGet() == TYP_BYREF) + else if ((genActualType(tree) == TYP_INT) && (tempDsc->TypeGet() == TYP_BYREF)) { // Spill clique has decided this should be "byref", but this block only pushes an "int". // Insert a sign-extension to "native int" so we match the clique size.