From: Pat Gavlin Date: Mon, 12 Dec 2016 19:34:26 +0000 (-0800) Subject: Fix consume-order checking in codegen. X-Git-Tag: submit/tizen/20210909.063632~11030^2~8609^2~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=49d5ee1814a6ee3ef30e26db07436a17752817e6;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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[];