[TODO-List-Cleanup] Refactor `fgDebugCheckFlags` (#62948)
authorSingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Tue, 11 Jan 2022 01:49:04 +0000 (04:49 +0300)
committerGitHub <noreply@github.com>
Tue, 11 Jan 2022 01:49:04 +0000 (17:49 -0800)
* Refactor "fgDebugCheckFlags" with "VisitOperands"

This change does away with one more case of custom traversal logic.

Semantic changes have mostly been avoided, with some exceptions:

1) Dead checking of GLOB_REF on ADDR was dropped
2) Dead checking of GLOB_REF on statics was commented out
3) REVERSE_OPS checking was strengthened
4) Flag propagation for calls was also strengthened

Note: the GLOB_REF checking was dead because we do not check for
"extra" GLOB_REF flags, and the code in question was using the
"actual" "treeFlags".

Also renamed "treeFlags" to "actualFlags" and "chkFlags" to
"expectedFlags" as the handle checking code was apparently
confused by the (somewhat unobvious) names and got them swapped.

* Fix flags copying in "gtCloneExpr"

The code in question was copying GT_LCL_VAR flags to GT_CNS_INT
nodes, which is dangerous and not necessary. There is no need to
copy the flags in case we didn't create a new constant node as
the code under "DONE:" already does that.

Found by the new checking code for GTF_REVERSE_OPS.

src/coreclr/jit/compiler.h
src/coreclr/jit/fgdiagnostic.cpp
src/coreclr/jit/gentree.cpp
src/coreclr/jit/gentree.h

index e195ba9..1ba86f6 100644 (file)
@@ -6010,7 +6010,7 @@ public:
 
     void fgDebugCheckFlags(GenTree* tree);
     void fgDebugCheckDispFlags(GenTree* tree, GenTreeFlags dispFlags, GenTreeDebugFlags debugFlags);
-    void fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags treeFlags, GenTreeFlags chkFlags);
+    void fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags actualFlags, GenTreeFlags expectedFlags);
     void fgDebugCheckTryFinallyExits();
     void fgDebugCheckProfileData();
     bool fgDebugCheckIncomingProfileData(BasicBlock* block);
index 83636f1..7fd7b5d 100644 (file)
@@ -2924,443 +2924,187 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef
     }
 }
 
-/*****************************************************************************
- *
- * A DEBUG routine to check the that the exception flags are correctly set.
- *
- ****************************************************************************/
-
+//------------------------------------------------------------------------
+// fgDebugCheckFlags: Validate various invariants related to the propagation
+//                    and setting of tree flags ("gtFlags").
+//
+// Arguments:
+//    tree - the tree to (recursively) check the flags for
+//
 void Compiler::fgDebugCheckFlags(GenTree* tree)
 {
-    const genTreeOps oper      = tree->OperGet();
-    const unsigned   kind      = tree->OperKind();
-    GenTreeFlags     treeFlags = tree->gtFlags & GTF_ALL_EFFECT;
-    GenTreeFlags     chkFlags  = GTF_EMPTY;
+    GenTreeFlags actualFlags   = tree->gtFlags & GTF_ALL_EFFECT;
+    GenTreeFlags expectedFlags = GTF_EMPTY;
 
     if (tree->OperMayThrow(this))
     {
-        chkFlags |= GTF_EXCEPT;
+        expectedFlags |= GTF_EXCEPT;
     }
 
     if (tree->OperRequiresAsgFlag())
     {
-        chkFlags |= GTF_ASG;
+        expectedFlags |= GTF_ASG;
     }
 
     if (tree->OperRequiresCallFlag(this))
     {
-        chkFlags |= GTF_CALL;
+        expectedFlags |= GTF_CALL;
     }
 
-    /* Is this a leaf node? */
-
-    if (kind & GTK_LEAF)
+    // We reuse GTF_REVERSE_OPS as GTF_VAR_ARR_INDEX for LCL_VAR nodes.
+    if (((tree->gtFlags & GTF_REVERSE_OPS) != 0) && !tree->OperIs(GT_LCL_VAR))
     {
-        switch (oper)
-        {
-            case GT_CLS_VAR:
-                chkFlags |= GTF_GLOB_REF;
-                break;
-
-            case GT_CATCH_ARG:
-                chkFlags |= GTF_ORDER_SIDEEFF;
-                break;
-
-            case GT_MEMORYBARRIER:
-                chkFlags |= GTF_GLOB_REF | GTF_ASG;
-                break;
-
-            case GT_LCL_VAR:
-                assert((tree->gtFlags & GTF_VAR_FOLDED_IND) == 0);
-                break;
-
-            default:
-                break;
-        }
+        assert(tree->OperSupportsReverseOps());
     }
 
-    /* Is it a 'simple' unary/binary operator? */
+    GenTree* op1 = tree->OperIsSimple() ? tree->gtGetOp1() : nullptr;
 
-    else if (kind & GTK_SMPOP)
+    switch (tree->OperGet())
     {
-        GenTree* op1 = tree->AsOp()->gtOp1;
-        GenTree* op2 = tree->gtGetOp2IfPresent();
+        case GT_CLS_VAR:
+            expectedFlags |= GTF_GLOB_REF;
+            break;
 
-        // During GS work, we make shadow copies for params.
-        // In gsParamsToShadows(), we create a shadow var of TYP_INT for every small type param.
-        // Then in gsReplaceShadowParams(), we change the gtLclNum to the shadow var.
-        // We also change the types of the local var tree and the assignment tree to TYP_INT if necessary.
-        // However, since we don't morph the tree at this late stage. Manually propagating
-        // TYP_INT up to the GT_ASG tree is only correct if we don't need to propagate the TYP_INT back up.
-        // The following checks will ensure this.
+        case GT_CATCH_ARG:
+            expectedFlags |= GTF_ORDER_SIDEEFF;
+            break;
 
-        // Is the left child of "tree" a GT_ASG?
-        //
-        // If parent is a TYP_VOID, we don't no need to propagate TYP_INT up. We are fine.
-        // (or) If GT_ASG is the left child of a GT_COMMA, the type of the GT_COMMA node will
-        // be determined by its right child. So we don't need to propagate TYP_INT up either. We are fine.
-        if (op1 && op1->gtOper == GT_ASG)
-        {
-            assert(tree->gtType == TYP_VOID || tree->gtOper == GT_COMMA);
-        }
+        case GT_MEMORYBARRIER:
+            expectedFlags |= (GTF_GLOB_REF | GTF_ASG);
+            break;
 
-        // Is the right child of "tree" a GT_ASG?
-        //
-        // If parent is a TYP_VOID, we don't no need to propagate TYP_INT up. We are fine.
-        if (op2 && op2->gtOper == GT_ASG)
-        {
-            // We can have ASGs on the RHS of COMMAs in setup arguments to a call.
-            assert(tree->gtType == TYP_VOID || tree->gtOper == GT_COMMA);
-        }
+        case GT_LCL_VAR:
+            assert((tree->gtFlags & GTF_VAR_FOLDED_IND) == 0);
+            break;
 
-        switch (oper)
-        {
-            case GT_QMARK:
-                if (op1->OperIsCompare())
+        case GT_QMARK:
+            assert(!op1->CanCSE());
+            assert(op1->OperIsCompare() || op1->IsIntegralConst(0) || op1->IsIntegralConst(1));
+            break;
+
+        case GT_ASG:
+        case GT_ADDR:
+            // Note that this is a weak check - the "op1" location node can be a COMMA.
+            assert(!op1->CanCSE());
+            break;
+
+        case GT_IND:
+            // Do we have a constant integer address as op1 that is also a handle?
+            if (op1->IsCnsIntOrI() && op1->IsIconHandle())
+            {
+                if ((tree->gtFlags & GTF_IND_INVARIANT) != 0)
                 {
-                    noway_assert(op1->gtFlags & GTF_DONT_CSE);
+                    actualFlags |= GTF_IND_INVARIANT;
                 }
-                else
+                if ((tree->gtFlags & GTF_IND_NONFAULTING) != 0)
                 {
-                    noway_assert((op1->gtOper == GT_CNS_INT) &&
-                                 ((op1->AsIntCon()->gtIconVal == 0) || (op1->AsIntCon()->gtIconVal == 1)));
+                    actualFlags |= GTF_IND_NONFAULTING;
                 }
-                break;
 
-            case GT_ADDR:
-                assert(!op1->CanCSE());
-                break;
+                GenTreeFlags handleKind = op1->GetIconHandleFlag();
 
-            case GT_IND:
-                // Do we have a constant integer address as op1?
-                //
-                if (op1->OperGet() == GT_CNS_INT)
+                // 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
                 {
-                    // Is this constant a handle of some kind?
-                    //
-                    GenTreeFlags 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       |
-                        //                 +--------------+----------------+
-                    }
+                    // For statics, we expect the GTF_GLOB_REF to be set. However, we currently
+                    // fail to set it in a number of situations, and so this check is disabled.
+                    // TODO: enable checking of GTF_GLOB_REF.
+                    // expectedFlags |= GTF_GLOB_REF;
+                }
+                else // All the other handle indirections are considered invariant
+                {
+                    expectedFlags |= GTF_IND_INVARIANT;
                 }
-                break;
 
-            case GT_ASG:
-            {
-                // Can't CSE dst.
-                assert((tree->gtGetOp1()->gtFlags & GTF_DONT_CSE) != 0);
-                break;
+                // Currently we expect all indirections with constant addresses to be nonfaulting.
+                expectedFlags |= GTF_IND_NONFAULTING;
             }
-            default:
-                break;
-        }
-
-        /* Recursively check the subtrees */
-
-        if (op1)
-        {
-            fgDebugCheckFlags(op1);
-        }
-        if (op2)
-        {
-            fgDebugCheckFlags(op2);
-        }
-
-        if (op1)
-        {
-            chkFlags |= (op1->gtFlags & GTF_ALL_EFFECT);
-        }
-        if (op2)
-        {
-            chkFlags |= (op2->gtFlags & GTF_ALL_EFFECT);
-        }
+            break;
 
-        // We reuse the value of GTF_REVERSE_OPS for a GT_IND-specific flag,
-        // so exempt that (unary) operator.
-        if (tree->OperGet() != GT_IND && tree->gtFlags & GTF_REVERSE_OPS)
-        {
-            /* Must have two operands if GTF_REVERSE is set */
-            noway_assert(op1 && op2);
+        case GT_CALL:
 
-            /* Make sure that the order of side effects has not been swapped. */
+            GenTreeCall* call;
 
-            /* However CSE may introduce an assignment after the reverse flag
-               was set and thus GTF_ASG cannot be considered here. */
+            call = tree->AsCall();
 
-            /* For a GT_ASG(GT_IND(x), y) we are interested in the side effects of x */
-            GenTree* op1p;
-            if ((oper == GT_ASG) && (op1->gtOper == GT_IND))
-            {
-                op1p = op1->AsOp()->gtOp1;
-            }
-            else
+            if ((call->gtCallThisArg != nullptr) && ((call->gtCallThisArg->GetNode()->gtFlags & GTF_ASG) != 0))
             {
-                op1p = op1;
+                // TODO-Cleanup: this is a patch for a violation in our GT_ASG propagation.
+                // see https://github.com/dotnet/runtime/issues/13758
+                actualFlags |= GTF_ASG;
             }
 
-            /* This isn't true any more with the sticky GTF_REVERSE */
-            /*
-            // if op1p has side effects, then op2 cannot have side effects
-            if (op1p->gtFlags & (GTF_SIDE_EFFECT & ~GTF_ASG))
+            for (GenTreeCall::Use& use : call->Args())
             {
-                if (op2->gtFlags & (GTF_SIDE_EFFECT & ~GTF_ASG))
-                    gtDispTree(tree);
-                noway_assert(!(op2->gtFlags & (GTF_SIDE_EFFECT & ~GTF_ASG)));
-            }
-            */
-        }
-
-        if (oper == GT_ADDR && (op1->OperIsLocal() || op1->gtOper == GT_CLS_VAR ||
-                                (op1->gtOper == GT_IND && op1->AsOp()->gtOp1->gtOper == GT_CLS_VAR_ADDR)))
-        {
-            /* &aliasedVar doesn't need GTF_GLOB_REF, though alisasedVar does.
-               Similarly for clsVar */
-            treeFlags |= GTF_GLOB_REF;
-        }
-    }
-
-    /* See what kind of a special operator we have here */
-
-    else
-    {
-        switch (tree->OperGet())
-        {
-            case GT_CALL:
-
-                GenTreeCall* call;
-
-                call = tree->AsCall();
-
-                if (call->gtCallThisArg != nullptr)
-                {
-                    fgDebugCheckFlags(call->gtCallThisArg->GetNode());
-                    chkFlags |= (call->gtCallThisArg->GetNode()->gtFlags & GTF_SIDE_EFFECT);
-
-                    if ((call->gtCallThisArg->GetNode()->gtFlags & GTF_ASG) != 0)
-                    {
-                        // TODO-Cleanup: this is a patch for a violation in our GT_ASG propagation
-                        // see https://github.com/dotnet/runtime/issues/13758
-                        treeFlags |= GTF_ASG;
-                    }
-                }
-
-                for (GenTreeCall::Use& use : call->Args())
-                {
-                    fgDebugCheckFlags(use.GetNode());
-
-                    chkFlags |= (use.GetNode()->gtFlags & GTF_SIDE_EFFECT);
-
-                    if ((use.GetNode()->gtFlags & GTF_ASG) != 0)
-                    {
-                        // TODO-Cleanup: this is a patch for a violation in our GT_ASG propagation
-                        // see https://github.com/dotnet/runtime/issues/13758
-                        treeFlags |= GTF_ASG;
-                    }
-                }
-
-                for (GenTreeCall::Use& use : call->LateArgs())
+                if ((use.GetNode()->gtFlags & GTF_ASG) != 0)
                 {
-                    fgDebugCheckFlags(use.GetNode());
-
-                    chkFlags |= (use.GetNode()->gtFlags & GTF_SIDE_EFFECT);
-
-                    if ((use.GetNode()->gtFlags & GTF_ASG) != 0)
-                    {
-                        treeFlags |= GTF_ASG;
-                    }
-                }
-
-                if ((call->gtCallType == CT_INDIRECT) && (call->gtCallCookie != nullptr))
-                {
-                    fgDebugCheckFlags(call->gtCallCookie);
-                    chkFlags |= (call->gtCallCookie->gtFlags & GTF_SIDE_EFFECT);
-                }
-
-                if (call->gtCallType == CT_INDIRECT)
-                {
-                    fgDebugCheckFlags(call->gtCallAddr);
-                    chkFlags |= (call->gtCallAddr->gtFlags & GTF_SIDE_EFFECT);
-                }
-
-                if ((call->gtControlExpr != nullptr) && call->IsExpandedEarly() && call->IsVirtualVtable())
-                {
-                    fgDebugCheckFlags(call->gtControlExpr);
-                    chkFlags |= (call->gtControlExpr->gtFlags & GTF_SIDE_EFFECT);
+                    // TODO-Cleanup: this is a patch for a violation in our GT_ASG propagation.
+                    // see https://github.com/dotnet/runtime/issues/13758
+                    actualFlags |= GTF_ASG;
                 }
+            }
 
-                if (call->IsUnmanaged() && (call->gtCallMoreFlags & GTF_CALL_M_UNMGD_THISCALL))
-                {
-                    if (call->gtCallArgs->GetNode()->OperGet() == GT_NOP)
-                    {
-                        noway_assert(call->gtCallLateArgs->GetNode()->TypeGet() == TYP_I_IMPL ||
-                                     call->gtCallLateArgs->GetNode()->TypeGet() == TYP_BYREF);
-                    }
-                    else
-                    {
-                        noway_assert(call->gtCallArgs->GetNode()->TypeGet() == TYP_I_IMPL ||
-                                     call->gtCallArgs->GetNode()->TypeGet() == TYP_BYREF);
-                    }
-                }
-                break;
-
-#if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)
-#if defined(FEATURE_SIMD)
-            case GT_SIMD:
-#endif
-#if defined(FEATURE_HW_INTRINSICS)
-            case GT_HWINTRINSIC:
-#endif
-                // TODO-List-Cleanup: consider using the general Operands() iterator
-                // here for the "special" nodes to reduce code duplication.
-                for (GenTree* operand : tree->AsMultiOp()->Operands())
+            for (GenTreeCall::Use& use : call->LateArgs())
+            {
+                if ((use.GetNode()->gtFlags & GTF_ASG) != 0)
                 {
-                    fgDebugCheckFlags(operand);
-                    chkFlags |= (operand->gtFlags & GTF_ALL_EFFECT);
+                    // TODO-Cleanup: this is a patch for a violation in our GT_ASG propagation.
+                    // see https://github.com/dotnet/runtime/issues/13758
+                    actualFlags |= GTF_ASG;
                 }
-                break;
-#endif // defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)
-
-            case GT_ARR_ELEM:
-
-                GenTree* arrObj;
-                unsigned dim;
-
-                arrObj = tree->AsArrElem()->gtArrObj;
-                fgDebugCheckFlags(arrObj);
-                chkFlags |= (arrObj->gtFlags & GTF_ALL_EFFECT);
+            }
 
-                for (dim = 0; dim < tree->AsArrElem()->gtArrRank; dim++)
+            if (call->IsUnmanaged() && ((call->gtCallMoreFlags & GTF_CALL_M_UNMGD_THISCALL) != 0))
+            {
+                if (call->gtCallArgs->GetNode()->OperGet() == GT_NOP)
                 {
-                    fgDebugCheckFlags(tree->AsArrElem()->gtArrInds[dim]);
-                    chkFlags |= tree->AsArrElem()->gtArrInds[dim]->gtFlags & GTF_ALL_EFFECT;
+                    assert(call->gtCallLateArgs->GetNode()->TypeIs(TYP_I_IMPL, TYP_BYREF));
                 }
-                break;
-
-            case GT_ARR_OFFSET:
-
-                fgDebugCheckFlags(tree->AsArrOffs()->gtOffset);
-                chkFlags |= (tree->AsArrOffs()->gtOffset->gtFlags & GTF_ALL_EFFECT);
-                fgDebugCheckFlags(tree->AsArrOffs()->gtIndex);
-                chkFlags |= (tree->AsArrOffs()->gtIndex->gtFlags & GTF_ALL_EFFECT);
-                fgDebugCheckFlags(tree->AsArrOffs()->gtArrObj);
-                chkFlags |= (tree->AsArrOffs()->gtArrObj->gtFlags & GTF_ALL_EFFECT);
-                break;
-
-            case GT_PHI:
-                for (GenTreePhi::Use& use : tree->AsPhi()->Uses())
+                else
                 {
-                    fgDebugCheckFlags(use.GetNode());
-                    chkFlags |= (use.GetNode()->gtFlags & GTF_ALL_EFFECT);
+                    assert(call->gtCallArgs->GetNode()->TypeIs(TYP_I_IMPL, TYP_BYREF));
                 }
-                break;
+            }
+            break;
 
-            case GT_FIELD_LIST:
-                for (GenTreeFieldList::Use& use : tree->AsFieldList()->Uses())
-                {
-                    fgDebugCheckFlags(use.GetNode());
-                    chkFlags |= (use.GetNode()->gtFlags & GTF_ALL_EFFECT);
-                }
-                break;
+        case GT_CMPXCHG:
+            expectedFlags |= (GTF_GLOB_REF | GTF_ASG);
+            break;
 
-            case GT_CMPXCHG:
-
-                chkFlags |= (GTF_GLOB_REF | GTF_ASG);
-                GenTreeCmpXchg* cmpXchg;
-                cmpXchg = tree->AsCmpXchg();
-                fgDebugCheckFlags(cmpXchg->gtOpLocation);
-                chkFlags |= (cmpXchg->gtOpLocation->gtFlags & GTF_ALL_EFFECT);
-                fgDebugCheckFlags(cmpXchg->gtOpValue);
-                chkFlags |= (cmpXchg->gtOpValue->gtFlags & GTF_ALL_EFFECT);
-                fgDebugCheckFlags(cmpXchg->gtOpComparand);
-                chkFlags |= (cmpXchg->gtOpComparand->gtFlags & GTF_ALL_EFFECT);
-                break;
+        default:
+            break;
+    }
 
-            case GT_STORE_DYN_BLK:
-            case GT_DYN_BLK:
+    tree->VisitOperands([&](GenTree* operand) -> GenTree::VisitResult {
 
-                GenTreeDynBlk* dynBlk;
-                dynBlk = tree->AsDynBlk();
-                fgDebugCheckFlags(dynBlk->gtDynamicSize);
-                chkFlags |= (dynBlk->gtDynamicSize->gtFlags & GTF_ALL_EFFECT);
-                fgDebugCheckFlags(dynBlk->Addr());
-                chkFlags |= (dynBlk->Addr()->gtFlags & GTF_ALL_EFFECT);
-                if (tree->OperGet() == GT_STORE_DYN_BLK)
-                {
-                    fgDebugCheckFlags(dynBlk->Data());
-                    chkFlags |= (dynBlk->Data()->gtFlags & GTF_ALL_EFFECT);
-                }
-                break;
+        // ASGs are nodes that produce no value, but have a type (essentially, the type of the location).
+        // Validate that nodes that parent ASGs do not consume values. This check also ensures that code
+        // which updates location types ("gsParamsToShadows" replaces small LCL_VARs with TYP_INT ones)
+        // does not have to worry about propagating the new type "up the tree".
+        //
+        // Uncoditionally allowing COMMA here weakens the assert, but is necessary because the compiler
+        // ("gtExtractSideEffList") can create "typed" "comma lists" with ASGs as second operands.
+        //
+        if (operand->OperIs(GT_ASG))
+        {
+            assert(tree->IsCall() || tree->OperIs(GT_COMMA));
+        }
 
-            default:
+        fgDebugCheckFlags(operand);
+        expectedFlags |= (operand->gtFlags & GTF_ALL_EFFECT);
 
-#ifdef DEBUG
-                gtDispTree(tree);
-#endif
+        return GenTree::VisitResult::Continue;
+    });
 
-                assert(!"Unknown operator for fgDebugCheckFlags");
-                break;
-        }
+    // ADDR nodes break the "parent flags >= operands flags" invariant for GTF_GLOB_REF.
+    if (tree->OperIs(GT_ADDR) && op1->OperIs(GT_LCL_VAR, GT_LCL_FLD, GT_CLS_VAR))
+    {
+        expectedFlags &= ~GTF_GLOB_REF;
     }
 
-    fgDebugCheckFlagsHelper(tree, treeFlags, chkFlags);
+    fgDebugCheckFlagsHelper(tree, actualFlags, expectedFlags);
 }
 
 //------------------------------------------------------------------------------
@@ -3388,20 +3132,17 @@ void Compiler::fgDebugCheckDispFlags(GenTree* tree, GenTreeFlags dispFlags, GenT
 // fgDebugCheckFlagsHelper : Check if all bits that are set in chkFlags are also set in treeFlags.
 //
 // Arguments:
-//    tree      - Tree whose flags are being checked
-//    treeFlags - Actual flags on the tree
-//    chkFlags  - Expected flags
-//
-// Note:
-//    Checking that all bits that are set in treeFlags are also set in chkFlags is currently disabled.
+//    tree          - Tree whose flags are being checked
+//    actualFlags   - Actual flags on the tree
+//    expectedFlags - Expected flags
 //
-void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags treeFlags, GenTreeFlags chkFlags)
+void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags actualFlags, GenTreeFlags expectedFlags)
 {
-    if (chkFlags & ~treeFlags)
+    if (expectedFlags & ~actualFlags)
     {
         // Print the tree so we can see it in the log.
         printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
-        Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE);
+        Compiler::fgDebugCheckDispFlags(tree, expectedFlags & ~actualFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
 
@@ -3409,21 +3150,21 @@ void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags treeFlags, Ge
 
         // Print the tree again so we can see it right after we hook up the debugger.
         printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
-        Compiler::fgDebugCheckDispFlags(tree, chkFlags & ~treeFlags, GTF_DEBUG_NONE);
+        Compiler::fgDebugCheckDispFlags(tree, expectedFlags & ~actualFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
     }
-    else if (treeFlags & ~chkFlags)
+    else if (actualFlags & ~expectedFlags)
     {
         // We can't/don't consider these flags (GTF_GLOB_REF or GTF_ORDER_SIDEEFF) as being "extra" flags
         //
         GenTreeFlags flagsToCheck = ~GTF_GLOB_REF & ~GTF_ORDER_SIDEEFF;
 
-        if ((treeFlags & ~chkFlags & flagsToCheck) != 0)
+        if ((actualFlags & ~expectedFlags & flagsToCheck) != 0)
         {
             // Print the tree so we can see it in the log.
             printf("Extra flags on tree [%06d]: ", dspTreeID(tree));
-            Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE);
+            Compiler::fgDebugCheckDispFlags(tree, actualFlags & ~expectedFlags, GTF_DEBUG_NONE);
             printf("\n");
             gtDispTree(tree);
 
@@ -3431,7 +3172,7 @@ void Compiler::fgDebugCheckFlagsHelper(GenTree* tree, GenTreeFlags treeFlags, Ge
 
             // Print the tree again so we can see it right after we hook up the debugger.
             printf("Extra flags on tree [%06d]: ", dspTreeID(tree));
-            Compiler::fgDebugCheckDispFlags(tree, treeFlags & ~chkFlags, GTF_DEBUG_NONE);
+            Compiler::fgDebugCheckDispFlags(tree, actualFlags & ~expectedFlags, GTF_DEBUG_NONE);
             printf("\n");
             gtDispTree(tree);
         }
index 5769db6..3a61e63 100644 (file)
@@ -7245,7 +7245,6 @@ GenTree* Compiler::gtCloneExpr(
                                          tree->gtType DEBUGARG(tree->AsLclVar()->gtLclILoffs));
                     copy->AsLclVarCommon()->SetSsaNum(tree->AsLclVarCommon()->GetSsaNum());
                 }
-                copy->gtFlags = tree->gtFlags;
                 goto DONE;
 
             case GT_LCL_FLD:
index 4355130..c7a0ef0 100644 (file)
@@ -1723,7 +1723,8 @@ public:
         }
     }
 
-    static inline bool RequiresNonNullOp2(genTreeOps oper);
+    bool        OperSupportsReverseOps() const;
+    static bool RequiresNonNullOp2(genTreeOps oper);
     bool IsValidCallArgument();
 #endif // DEBUG
 
@@ -7747,8 +7748,7 @@ inline GenTree* GenTree::gtGetOp1() const
 }
 
 #ifdef DEBUG
-/* static */
-inline bool GenTree::RequiresNonNullOp2(genTreeOps oper)
+/* static */ inline bool GenTree::RequiresNonNullOp2(genTreeOps oper)
 {
     switch (oper)
     {
@@ -7784,6 +7784,21 @@ inline bool GenTree::RequiresNonNullOp2(genTreeOps oper)
             return false;
     }
 }
+
+inline bool GenTree::OperSupportsReverseOps() const
+{
+    if (OperIsBinary() && !OperIs(GT_COMMA, GT_INTRINSIC, GT_BOUNDS_CHECK))
+    {
+        return (AsOp()->gtGetOp1() != nullptr) && (AsOp()->gtGetOp2() != nullptr);
+    }
+#if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS)
+    if (OperIsMultiOp())
+    {
+        return AsMultiOp()->GetOperandCount() == 2;
+    }
+#endif // FEATURE_SIMD || FEATURE_HW_INTRINSICS
+    return false;
+}
 #endif // DEBUG
 
 inline GenTree* GenTree::gtGetOp2() const