From 08053aef66b1bf1d61de4ee744181ec265718327 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Fri, 15 Sep 2017 17:13:03 -0700 Subject: [PATCH] [RyuJit] fix the inconsistency between setContained and isContained. (#13991) * show the problem with contained arg_place We set contained on PUTARG_REG, but it doesn't pass IsContained check. * Fix problem with gtControlExpr * fix problem with ARGPLACE * additional improvements1 We should never have a contained node that is the last node in the execution order. * additional impovement2 for xarch. It is redundant, do not need to set as contained. * additional improvement2 for arm `GenTree* ctrlExpr = call->gtControlExpr;` was unused. * additional improvement3: unify CheckLir. --- src/jit/gentree.cpp | 16 +++++++++++++--- src/jit/gtlist.h | 2 +- src/jit/lir.cpp | 10 +++------- src/jit/lower.cpp | 1 + src/jit/lowerarmarch.cpp | 17 +---------------- src/jit/lowerxarch.cpp | 17 +---------------- 6 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index ff7372c..bcf8d36 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -15605,9 +15605,19 @@ bool GenTree::canBeContained() const { assert(IsLIR()); + if (gtHasReg()) + { + return false; + } + // It is not possible for nodes that do not produce values or that are not containable values // to be contained. - return (OperKind() & (GTK_NOVALUE | GTK_NOCONTAIN)) == 0; + if ((OperKind() & (GTK_NOVALUE | GTK_NOCONTAIN)) != 0) + { + return false; + } + + return true; } //------------------------------------------------------------------------ @@ -15632,7 +15642,7 @@ bool GenTree::isContained() const const bool isMarkedContained = ((gtFlags & GTF_CONTAINED) != 0); #ifdef DEBUG - if (!canBeContained() || gtHasReg()) + if (!canBeContained()) { assert(!isMarkedContained); } @@ -15666,7 +15676,7 @@ bool GenTree::isContained() const // if it's contained it better have a user if (isMarkedContained) { - assert((gtNext != nullptr) || OperIsLocal()); + assert(gtNext != nullptr); } #endif // DEBUG return isMarkedContained; diff --git a/src/jit/gtlist.h b/src/jit/gtlist.h index 314660f..b9098da 100644 --- a/src/jit/gtlist.h +++ b/src/jit/gtlist.h @@ -292,7 +292,7 @@ GTNODE(SWITCH_TABLE , GenTreeOp ,0, GTK_BINOP|GTK_NOVALUE) // Jump GTNODE(REG_VAR , GenTreeLclVar ,0,GTK_LEAF|GTK_LOCAL) // register variable GTNODE(CLS_VAR , GenTreeClsVar ,0,GTK_LEAF) // static data member GTNODE(CLS_VAR_ADDR , GenTreeClsVar ,0,GTK_LEAF) // static data member address -GTNODE(ARGPLACE , GenTreeArgPlace ,0,GTK_LEAF) // placeholder for a register arg +GTNODE(ARGPLACE , GenTreeArgPlace ,0,GTK_LEAF|GTK_NOVALUE) // placeholder for a register arg GTNODE(NULLCHECK , GenTreeOp ,0,GTK_UNOP|GTK_NOVALUE) // null checks the source GTNODE(PHYSREG , GenTreePhysReg ,0,GTK_LEAF) // read from a physical register GTNODE(EMITNOP , GenTree ,0,GTK_LEAF|GTK_NOVALUE) // emitter-placed nop diff --git a/src/jit/lir.cpp b/src/jit/lir.cpp index ecfe6c5..02ba714 100644 --- a/src/jit/lir.cpp +++ b/src/jit/lir.cpp @@ -1501,20 +1501,16 @@ bool LIR::Range::CheckLIR(Compiler* compiler, bool checkUnusedValues) const assert(!(checkUnusedValues && def->IsUnusedValue()) && "operands should never be marked as unused values"); - if (def->OperGet() == GT_ARGPLACE) - { - // ARGPLACE nodes are not represented in the LIR sequence. Ignore them. - continue; - } - else if (!def->IsValue()) + if (!def->IsValue()) { // Stack arguments do not produce a value, but they are considered children of the call. // It may be useful to remove these from being call operands, but that may also impact // other code that relies on being able to reach all the operands from a call node. // The GT_NOP case is because sometimes we eliminate stack argument stores as dead, but // instead of removing them we replace with a NOP. + // ARGPLACE nodes are not represented in the LIR sequence. Ignore them. assert((node->OperGet() == GT_CALL) && - (def->OperIsStore() || (def->OperGet() == GT_PUTARG_STK) || (def->OperGet() == GT_NOP))); + (def->OperIsStore() || def->OperIs(GT_PUTARG_STK, GT_NOP, GT_ARGPLACE))); continue; } diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp index 3c8a79d..6bb1c40 100644 --- a/src/jit/lower.cpp +++ b/src/jit/lower.cpp @@ -44,6 +44,7 @@ void Lowering::MakeSrcContained(GenTreePtr parentNode, GenTreePtr childNode) assert(!parentNode->OperIsLeaf()); assert(childNode->canBeContained()); childNode->SetContained(); + assert(childNode->isContained()); } //------------------------------------------------------------------------ diff --git a/src/jit/lowerarmarch.cpp b/src/jit/lowerarmarch.cpp index 64c7886..41de382 100644 --- a/src/jit/lowerarmarch.cpp +++ b/src/jit/lowerarmarch.cpp @@ -506,22 +506,7 @@ void Lowering::LowerRotate(GenTreePtr tree) // void Lowering::ContainCheckCallOperands(GenTreeCall* call) { - GenTree* ctrlExpr = call->gtControlExpr; - // If there is an explicit this pointer, we don't want that node to produce anything - // as it is redundant - if (call->gtCallObjp != nullptr) - { - GenTreePtr thisPtrNode = call->gtCallObjp; - - if (thisPtrNode->canBeContained()) - { - MakeSrcContained(call, thisPtrNode); - if (thisPtrNode->gtOper == GT_PUTARG_REG) - { - MakeSrcContained(call, thisPtrNode->gtOp.gtOp1); - } - } - } + // There are no contained operands for arm. } //------------------------------------------------------------------------ diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index 3240711..67a6d05 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -1326,25 +1326,10 @@ void Lowering::ContainCheckCallOperands(GenTreeCall* call) #endif // _TARGET_X86_ if (ctrlExpr->isIndir()) { - MakeSrcContained(call, ctrlExpr); // We may have cases where we have set a register target on the ctrlExpr, but if it // contained we must clear it. ctrlExpr->gtRegNum = REG_NA; - } - } - } - // If there is an explicit this pointer, we don't want that node to produce anything - // as it is redundant - if (call->gtCallObjp != nullptr) - { - GenTreePtr thisPtrNode = call->gtCallObjp; - - if (thisPtrNode->canBeContained()) - { - MakeSrcContained(call, thisPtrNode); - if (thisPtrNode->gtOper == GT_PUTARG_REG) - { - MakeSrcContained(call, thisPtrNode->gtOp.gtOp1); + MakeSrcContained(call, ctrlExpr); } } } -- 2.7.4