From: Sergey Andreenko Date: Thu, 30 Jul 2020 02:33:37 +0000 (-0700) Subject: Fix dummy OBJ/BLK/IND nodes. (#39824) X-Git-Tag: submit/tizen/20210909.063632~6332 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=ad90f7ab81b99fab7ca4891c3943021827e66315;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix dummy OBJ/BLK/IND nodes. (#39824) * Add a repro. * Add a helper function `fgTryRemoveNonLocal`. * Fix the issue. * extract `gtChangeOperToNullCheck`. * update comments. --- diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index cb129b3..4e1140e 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -9285,3 +9285,19 @@ bool Compiler::lvaIsOSRLocal(unsigned varNum) return false; } + +//------------------------------------------------------------------------------ +// gtChangeOperToNullCheck: helper to change tree oper to a NULLCHECK. +// +// Arguments: +// tree - the node to change; +// basicBlock - basic block of the node. +// +void Compiler::gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block) +{ + assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK)); + tree->ChangeOper(GT_NULLCHECK); + tree->ChangeType(TYP_BYTE); + block->bbFlags |= BBF_HAS_NULLCHECK; + optMethodFlags |= OMF_HAS_NULLCHECK; +} diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 011a2af..183e84c 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -2701,6 +2701,8 @@ public: GenTree* gtNewNullCheck(GenTree* addr, BasicBlock* basicBlock); + void gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block); + GenTreeArgList* gtNewArgList(GenTree* op); GenTreeArgList* gtNewArgList(GenTree* op1, GenTree* op2); GenTreeArgList* gtNewArgList(GenTree* op1, GenTree* op2, GenTree* op3); @@ -4633,6 +4635,8 @@ public: void fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP volatileVars); + bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange); + bool fgRemoveDeadStore(GenTree** pTree, LclVarDsc* varDsc, VARSET_VALARG_TP life, diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index ca5c6d4..2e0e96a 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -13782,16 +13782,13 @@ GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions // For struct types read the first byte of the // source struct; there's no need to read the // entire thing, and no place to put it. - assert(copySrc->gtOper == GT_OBJ || copySrc->gtOper == GT_IND || copySrc->gtOper == GT_FIELD); + assert(copySrc->OperIs(GT_OBJ, GT_IND, GT_FIELD)); copyStmt->SetRootNode(copySrc); if (options == BR_REMOVE_AND_NARROW || options == BR_REMOVE_AND_NARROW_WANT_TYPE_HANDLE) { JITDUMP(" to read first byte of struct via modified [%06u]\n", dspTreeID(copySrc)); - copySrc->ChangeOper(GT_NULLCHECK); - copySrc->gtType = TYP_BYTE; - compCurBB->bbFlags |= BBF_HAS_NULLCHECK; - optMethodFlags |= OMF_HAS_NULLCHECK; + gtChangeOperToNullCheck(copySrc, compCurBB); } else { @@ -15970,6 +15967,11 @@ void Compiler::gtExtractSideEffList(GenTree* expr, if (m_compiler->gtNodeHasSideEffects(node, m_flags)) { m_sideEffects.Push(node); + if (node->OperIsBlk() && !node->OperIsStoreBlk()) + { + JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node)); + m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB); + } return Compiler::WALK_SKIP_SUBTREES; } diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index 9bb5315..d111158 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -13247,10 +13247,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // via an underlying address, just null check the address. if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ)) { - op1->ChangeOper(GT_NULLCHECK); - block->bbFlags |= BBF_HAS_NULLCHECK; - optMethodFlags |= OMF_HAS_NULLCHECK; - op1->gtType = TYP_BYTE; + gtChangeOperToNullCheck(op1, block); } else { diff --git a/src/coreclr/src/jit/liveness.cpp b/src/coreclr/src/jit/liveness.cpp index feb8f23..504a6c0 100644 --- a/src/coreclr/src/jit/liveness.cpp +++ b/src/coreclr/src/jit/liveness.cpp @@ -2109,40 +2109,77 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR break; case GT_NOP: + { // NOTE: we need to keep some NOPs around because they are referenced by calls. See the dead store // removal code above (case GT_STORE_LCL_VAR) for more explanation. if ((node->gtFlags & GTF_ORDER_SIDEEFF) != 0) { break; } - __fallthrough; + fgTryRemoveNonLocal(node, &blockRange); + } + break; - default: - assert(!node->OperIsLocal()); - if (!node->IsValue() || node->IsUnusedValue()) + case GT_BLK: + case GT_OBJ: + case GT_DYN_BLK: + { + bool removed = fgTryRemoveNonLocal(node, &blockRange); + if (!removed && node->IsUnusedValue()) { - // We are only interested in avoiding the removal of nodes with direct side-effects - // (as opposed to side effects of their children). - // This default case should never include calls or assignments. - assert(!node->OperRequiresAsgFlag() && !node->OperIs(GT_CALL)); - if (!node->gtSetFlags() && !node->OperMayThrow(this)) - { - JITDUMP("Removing dead node:\n"); - DISPNODE(node); - - node->VisitOperands([](GenTree* operand) -> GenTree::VisitResult { - operand->SetUnusedValue(); - return GenTree::VisitResult::Continue; - }); - - blockRange.Remove(node); - } + // IR doesn't expect dummy uses of `GT_OBJ/BLK/DYN_BLK`. + JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node)); + gtChangeOperToNullCheck(node, block); } + } + break; + + default: + fgTryRemoveNonLocal(node, &blockRange); break; } } } +//--------------------------------------------------------------------- +// fgTryRemoveNonLocal - try to remove a node if it is unused and has no direct +// side effects. +// +// Arguments +// node - the non-local node to try; +// blockRange - the block range that contains the node. +// +// Return value: +// None +// +// Notes: local nodes are processed independently and are not expected in this function. +// +bool Compiler::fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange) +{ + assert(!node->OperIsLocal()); + if (!node->IsValue() || node->IsUnusedValue()) + { + // We are only interested in avoiding the removal of nodes with direct side effects + // (as opposed to side effects of their children). + // This default case should never include calls or assignments. + assert(!node->OperRequiresAsgFlag() && !node->OperIs(GT_CALL)); + if (!node->gtSetFlags() && !node->OperMayThrow(this)) + { + JITDUMP("Removing dead node:\n"); + DISPNODE(node); + + node->VisitOperands([](GenTree* operand) -> GenTree::VisitResult { + operand->SetUnusedValue(); + return GenTree::VisitResult::Continue; + }); + + blockRange->Remove(node); + return true; + } + } + return false; +} + // fgRemoveDeadStore - remove a store to a local which has no exposed uses. // // pTree - GenTree** to local, including store-form local or local addr (post-rationalize) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs new file mode 100644 index 0000000..2264049 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// The test was deleting the hardware intrinsic leaving unconsumed GT_OBJ on top of the stack +// that was leading to an assert failure. + +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.X86; +using System; + +class Runtime_39403 +{ + public static int Main() + { + if (Sse41.IsSupported) + { + Vector128 left = Vector128.Create(1); + Vector128 right = Vector128.Create(2); + ref var rightRef = ref right; + Vector128 mask = Vector128.Create(3); + Sse41.BlendVariable(left, rightRef, mask); + } + return 100; + } +} + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj new file mode 100644 index 0000000..5d49e8d --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj @@ -0,0 +1,13 @@ + + + Exe + + + + True + True + + + + +