Fix consume-order checking in codegen.

The switch to LIR invalidated the correspondence between a node's
sequence number and the order in which it must be consumed with respect
to other nodes. This in turn made the consume-order checks during code
generation incorrect.

This change introduces a new field, `gtUseNum`, that is used during code
generation to check the order in which nodes are consumed. Re-enabling
these checks revealed a bug in code generation for locked instructions
on x86, which were consuming their operands out-of-order; this change
also contains a fix for that bug.

Fixes dotnet/coreclr#7963.

Commit migrated from https://github.com/dotnet/coreclr/commit/adf5d7dc89676534a4b6a31ab3fcfcd0fd2d7f16

diff --git a/src/coreclr/src/jit/codegen.h b/src/coreclr/src/jit/codegen.h
index b6c8c18..c6e38ab 100755 (executable)
--- a/src/coreclr/src/jit/codegen.h
+++ b/src/coreclr/src/jit/codegen.h
@@ -122,7 +122,7 @@ private:
 
     void genRangeCheck(GenTree* node);
 
-    void genLockedInstructions(GenTree* node);
+    void genLockedInstructions(GenTreeOp* node);
 
     //-------------------------------------------------------------------------
     // Register-related methods

diff --git a/src/coreclr/src/jit/codegenarm64.cpp b/src/coreclr/src/jit/codegenarm64.cpp
index 5220ebc..8192fa1 100644 (file)
--- a/src/coreclr/src/jit/codegenarm64.cpp
+++ b/src/coreclr/src/jit/codegenarm64.cpp
@@ -2731,7 +2731,7 @@ void CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
         case GT_LOCKADD:
         case GT_XCHG:
         case GT_XADD:
-            genLockedInstructions(treeNode);
+            genLockedInstructions(treeNode->AsOp());
             break;
 
         case GT_MEMORYBARRIER:
@@ -3718,7 +3718,7 @@ void CodeGen::genJumpTable(GenTree* treeNode)
 
 // generate code for the locked operations:
 // GT_LOCKADD, GT_XCHG, GT_XADD
-void CodeGen::genLockedInstructions(GenTree* treeNode)
+void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
 {
 #if 0
     GenTree* data       = treeNode->gtOp.gtOp2;

diff --git a/src/coreclr/src/jit/codegenlinear.cpp b/src/coreclr/src/jit/codegenlinear.cpp
index 1cff163..752446e 100644 (file)
--- a/src/coreclr/src/jit/codegenlinear.cpp
+++ b/src/coreclr/src/jit/codegenlinear.cpp
@@ -313,13 +313,6 @@ void CodeGen::genCodeForBBlist()
 
         bool firstMapping = true;
 
-        /*---------------------------------------------------------------------
-         *
-         *  Generate code for each statement-tree in the block
-         *
-         */
-        CLANG_FORMAT_COMMENT_ANCHOR;
-
 #if FEATURE_EH_FUNCLETS
         if (block->bbFlags & BBF_FUNCLET_BEG)
         {
@@ -335,6 +328,28 @@ void CodeGen::genCodeForBBlist()
         // as we encounter it.
         CLANG_FORMAT_COMMENT_ANCHOR;
 
+#ifdef DEBUG
+        // Set the use-order numbers for each node.
+        {
+            int useNum = 0;
+            for (GenTree* node : LIR::AsRange(block).NonPhiNodes())
+            {
+                assert((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) == 0);
+
+                node->gtUseNum = -1;
+                if (node->isContained() || node->IsCopyOrReload())
+                {
+                    continue;
+                }
+
+                for (GenTree* operand : node->Operands())
+                {
+                    genNumberOperandUse(operand, useNum);
+                }
+            }
+        }
+#endif // DEBUG
+
         IL_OFFSETX currentILOffset = BAD_IL_OFFSET;
         for (GenTree* node : LIR::AsRange(block).NonPhiNodes())
         {
@@ -1031,34 +1046,55 @@ void CodeGen::genConsumeRegAndCopy(GenTree* node, regNumber needReg)
 
 // Check that registers are consumed in the right order for the current node being generated.
 #ifdef DEBUG
-void CodeGen::genCheckConsumeNode(GenTree* treeNode)
+void CodeGen::genNumberOperandUse(GenTree* const operand, int& useNum) const
 {
-    // GT_PUTARG_REG is consumed out of order.
-    if (treeNode->gtSeqNum != 0 && treeNode->OperGet() != GT_PUTARG_REG)
+    assert(operand != nullptr);
+    assert(operand->gtUseNum == -1);
+
+    // Ignore argument placeholders.
+    if (operand->OperGet() == GT_ARGPLACE)
     {
-        if (lastConsumedNode != nullptr)
+        return;
+    }
+
+    if (!operand->isContained() && !operand->IsCopyOrReload())
+    {
+        operand->gtUseNum = useNum;
+        useNum++;
+    }
+    else
+    {
+        for (GenTree* operand : operand->Operands())
         {
-            if (treeNode == lastConsumedNode)
-            {
-                if (verbose)
-                {
-                    printf("Node was consumed twice:\n    ");
-                    compiler->gtDispTree(treeNode, nullptr, nullptr, true);
-                }
-            }
-            else
-            {
-                if (verbose && (lastConsumedNode->gtSeqNum > treeNode->gtSeqNum))
-                {
-                    printf("Nodes were consumed out-of-order:\n");
-                    compiler->gtDispTree(lastConsumedNode, nullptr, nullptr, true);
-                    compiler->gtDispTree(treeNode, nullptr, nullptr, true);
-                }
-                // assert(lastConsumedNode->gtSeqNum < treeNode->gtSeqNum);
-            }
+            genNumberOperandUse(operand, useNum);
+        }
+    }
+}
+
+void CodeGen::genCheckConsumeNode(GenTree* const node)
+{
+    assert(node != nullptr);
+
+    if (verbose)
+    {
+        if ((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) != 0)
+        {
+            printf("Node was consumed twice:\n");
+            compiler->gtDispTree(node, nullptr, nullptr, true);
+        }
+        else if ((lastConsumedNode != nullptr) && (node->gtUseNum < lastConsumedNode->gtUseNum))
+        {
+            printf("Nodes were consumed out-of-order:\n");
+            compiler->gtDispTree(lastConsumedNode, nullptr, nullptr, true);
+            compiler->gtDispTree(node, nullptr, nullptr, true);
         }
-        lastConsumedNode = treeNode;
     }
+
+    assert((node->OperGet() == GT_CATCH_ARG) || ((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) == 0));
+    assert((lastConsumedNode == nullptr) || (node->gtUseNum == -1) || (node->gtUseNum > lastConsumedNode->gtUseNum));
+
+    node->gtDebugFlags |= GTF_DEBUG_NODE_CG_CONSUMED;
+    lastConsumedNode = node;
 }
 #endif // DEBUG
 

diff --git a/src/coreclr/src/jit/codegenlinear.h b/src/coreclr/src/jit/codegenlinear.h
index 25ad3f5..dbb8db9 100644 (file)
--- a/src/coreclr/src/jit/codegenlinear.h
+++ b/src/coreclr/src/jit/codegenlinear.h
@@ -253,7 +253,8 @@ unsigned m_stkArgOffset;
 
 #ifdef DEBUG
 GenTree* lastConsumedNode;
-void genCheckConsumeNode(GenTree* treeNode);
+void genNumberOperandUse(GenTree* const operand, int& useNum) const;
+void genCheckConsumeNode(GenTree* const node);
 #else  // !DEBUG
 inline void genCheckConsumeNode(GenTree* treeNode)
 {

diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp
index aa2484c..522b7c3 100644 (file)
--- a/src/coreclr/src/jit/codegenxarch.cpp
+++ b/src/coreclr/src/jit/codegenxarch.cpp
@@ -1997,7 +1997,7 @@ void CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
         case GT_LOCKADD:
         case GT_XCHG:
         case GT_XADD:
-            genLockedInstructions(treeNode);
+            genLockedInstructions(treeNode->AsOp());
             break;
 
         case GT_MEMORYBARRIER:
@@ -3786,7 +3786,7 @@ void CodeGen::genJumpTable(GenTree* treeNode)
 
 // generate code for the locked operations:
 // GT_LOCKADD, GT_XCHG, GT_XADD
-void CodeGen::genLockedInstructions(GenTree* treeNode)
+void CodeGen::genLockedInstructions(GenTreeOp* treeNode)
 {
     GenTree*    data      = treeNode->gtOp.gtOp2;
     GenTree*    addr      = treeNode->gtOp.gtOp1;
@@ -3795,11 +3795,6 @@ void CodeGen::genLockedInstructions(GenTree* treeNode)
     regNumber   addrReg   = addr->gtRegNum;
     instruction ins;
 
-    // all of these nodes implicitly do an indirection on op1
-    // so create a temporary node to feed into the pattern matching
-    GenTreeIndir i = indirForm(data->TypeGet(), addr);
-    genConsumeReg(addr);
-
     // The register allocator should have extended the lifetime of the address
     // so that it is not used as the target.
     noway_assert(addrReg != targetReg);
@@ -3809,7 +3804,7 @@ void CodeGen::genLockedInstructions(GenTree* treeNode)
     assert(targetReg != REG_NA || treeNode->OperGet() == GT_LOCKADD || !genIsRegCandidateLocal(data) ||
            (data->gtFlags & GTF_VAR_DEATH) != 0);
 
-    genConsumeIfReg(data);
+    genConsumeOperands(treeNode);
     if (targetReg != REG_NA && dataReg != REG_NA && dataReg != targetReg)
     {
         inst_RV_RV(ins_Copy(data->TypeGet()), targetReg, dataReg);
@@ -3835,6 +3830,10 @@ void CodeGen::genLockedInstructions(GenTree* treeNode)
         default:
             unreached();
     }
+
+    // all of these nodes implicitly do an indirection on op1
+    // so create a temporary node to feed into the pattern matching
+    GenTreeIndir i = indirForm(data->TypeGet(), addr);
     getEmitter()->emitInsBinary(ins, emitTypeSize(data), &i, data);
 
     if (treeNode->gtRegNum != REG_NA)

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
index 6518f91..8d5226e 100644 (file)
--- a/src/coreclr/src/jit/gentree.h
+++ b/src/coreclr/src/jit/gentree.h
@@ -968,8 +968,9 @@ public:
 #define GTF_DEBUG_NODE_SMALL 0x00000002
 #define GTF_DEBUG_NODE_LARGE 0x00000004
 #define GTF_DEBUG_NODE_CG_PRODUCED 0x00000008 // genProduceReg has been called on this node
+#define GTF_DEBUG_NODE_CG_CONSUMED 0x00000010 // genConsumeReg has been called on this node
 
-#define GTF_DEBUG_NODE_MASK 0x0000000F // These flags are all node (rather than operation) properties.
+#define GTF_DEBUG_NODE_MASK 0x0000001F // These flags are all node (rather than operation) properties.
 
 #define GTF_DEBUG_VAR_CSE_REF 0x00800000 // GT_LCL_VAR -- This is a CSE LCL_VAR node
 #endif                                   // defined(DEBUG)
@@ -980,6 +981,8 @@ public:
 #ifdef DEBUG
     unsigned gtTreeID;
     unsigned gtSeqNum; // liveness traversal order within the current statement
+
+    int gtUseNum; // use-ordered traversal within the function
 #endif
 
     static const unsigned short gtOperKindTable[];