[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.

diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp
index ff7372c..bcf8d36 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
             }
         }
     }