From e54081088e82813b5ab36d3a797172f3455c68ba Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Fri, 18 Jun 2010 01:24:29 +0000 Subject: [PATCH] Don't maintain a set of deleted nodes; instead, use a HandleSDNode to track a node over CSE events. This fixes PR7368. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@106266 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/X86/X86ISelDAGToDAG.cpp | 84 +++++++++----------------- test/CodeGen/X86/2010-03-17-ISelBug.ll | 28 +++++++++ 2 files changed, 57 insertions(+), 55 deletions(-) diff --git a/lib/Target/X86/X86ISelDAGToDAG.cpp b/lib/Target/X86/X86ISelDAGToDAG.cpp index 492a522e9a7..8bdd12329fb 100644 --- a/lib/Target/X86/X86ISelDAGToDAG.cpp +++ b/lib/Target/X86/X86ISelDAGToDAG.cpp @@ -137,21 +137,6 @@ namespace { } namespace { - class X86ISelListener : public SelectionDAG::DAGUpdateListener { - SmallSet Deletes; - public: - explicit X86ISelListener() {} - virtual void NodeDeleted(SDNode *N, SDNode *E) { - Deletes.insert(N); - } - virtual void NodeUpdated(SDNode *N) { - // Ignore updates. - } - bool IsDeleted(SDNode *N) { - return Deletes.count(N); - } - }; - //===--------------------------------------------------------------------===// /// ISel - X86 specific code to select X86 machine instructions for /// SelectionDAG operations. @@ -199,7 +184,6 @@ namespace { bool MatchWrapper(SDValue N, X86ISelAddressMode &AM); bool MatchAddress(SDValue N, X86ISelAddressMode &AM); bool MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, - X86ISelListener &DeadNodes, unsigned Depth); bool MatchAddressBase(SDValue N, X86ISelAddressMode &AM); bool SelectAddr(SDNode *Op, SDValue N, SDValue &Base, @@ -664,8 +648,7 @@ bool X86DAGToDAGISel::MatchWrapper(SDValue N, X86ISelAddressMode &AM) { /// returning true if it cannot be done. This just pattern matches for the /// addressing mode. bool X86DAGToDAGISel::MatchAddress(SDValue N, X86ISelAddressMode &AM) { - X86ISelListener DeadNodes; - if (MatchAddressRecursively(N, AM, DeadNodes, 0)) + if (MatchAddressRecursively(N, AM, 0)) return true; // Post-processing: Convert lea(,%reg,2) to lea(%reg,%reg), which has @@ -713,7 +696,6 @@ static bool isLogicallyAddWithConstant(SDValue V, SelectionDAG *CurDAG) { } bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, - X86ISelListener &DeadNodes, unsigned Depth) { bool is64Bit = Subtarget->is64Bit(); DebugLoc dl = N.getDebugLoc(); @@ -876,13 +858,13 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, // other uses, since it avoids a two-address sub instruction, however // it costs an additional mov if the index register has other uses. + // Add an artificial use to this node so that we can keep track of + // it if it gets CSE'd with a different node. + HandleSDNode Handle(N); + // Test if the LHS of the sub can be folded. X86ISelAddressMode Backup = AM; - if (MatchAddressRecursively(N.getNode()->getOperand(0), AM, - DeadNodes, Depth+1) || - // If it is successful but the recursive update causes N to be deleted, - // then it's not safe to continue. - DeadNodes.IsDeleted(N.getNode())) { + if (MatchAddressRecursively(N.getNode()->getOperand(0), AM, Depth+1)) { AM = Backup; break; } @@ -893,7 +875,7 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, } int Cost = 0; - SDValue RHS = N.getNode()->getOperand(1); + SDValue RHS = Handle.getValue().getNode()->getOperand(1); // If the RHS involves a register with multiple uses, this // transformation incurs an extra mov, due to the neg instruction // clobbering its operand. @@ -944,35 +926,27 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, } case ISD::ADD: { + // Add an artificial use to this node so that we can keep track of + // it if it gets CSE'd with a different node. + HandleSDNode Handle(N); + SDValue LHS = Handle.getValue().getNode()->getOperand(0); + SDValue RHS = Handle.getValue().getNode()->getOperand(1); + X86ISelAddressMode Backup = AM; - if (!MatchAddressRecursively(N.getNode()->getOperand(0), AM, - DeadNodes, Depth+1)) { - if (DeadNodes.IsDeleted(N.getNode())) - // If it is successful but the recursive update causes N to be deleted, - // then it's not safe to continue. - return true; - if (!MatchAddressRecursively(N.getNode()->getOperand(1), AM, - DeadNodes, Depth+1)) - // If it is successful but the recursive update causes N to be deleted, - // then it's not safe to continue. - return DeadNodes.IsDeleted(N.getNode()); - } + if (!MatchAddressRecursively(LHS, AM, Depth+1) && + !MatchAddressRecursively(RHS, AM, Depth+1)) + return false; + AM = Backup; + LHS = Handle.getValue().getNode()->getOperand(0); + RHS = Handle.getValue().getNode()->getOperand(1); // Try again after commuting the operands. + if (!MatchAddressRecursively(RHS, AM, Depth+1) && + !MatchAddressRecursively(LHS, AM, Depth+1)) + return false; AM = Backup; - if (!MatchAddressRecursively(N.getNode()->getOperand(1), AM, - DeadNodes, Depth+1)) { - if (DeadNodes.IsDeleted(N.getNode())) - // If it is successful but the recursive update causes N to be deleted, - // then it's not safe to continue. - return true; - if (!MatchAddressRecursively(N.getNode()->getOperand(0), AM, - DeadNodes, Depth+1)) - // If it is successful but the recursive update causes N to be deleted, - // then it's not safe to continue. - return DeadNodes.IsDeleted(N.getNode()); - } - AM = Backup; + LHS = Handle.getValue().getNode()->getOperand(0); + RHS = Handle.getValue().getNode()->getOperand(1); // If we couldn't fold both operands into the address at the same time, // see if we can just put each operand into a register and fold at least @@ -980,8 +954,8 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, if (AM.BaseType == X86ISelAddressMode::RegBase && !AM.Base_Reg.getNode() && !AM.IndexReg.getNode()) { - AM.Base_Reg = N.getNode()->getOperand(0); - AM.IndexReg = N.getNode()->getOperand(1); + AM.Base_Reg = LHS; + AM.IndexReg = RHS; AM.Scale = 1; return false; } @@ -996,7 +970,7 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, uint64_t Offset = CN->getSExtValue(); // Start with the LHS as an addr mode. - if (!MatchAddressRecursively(N.getOperand(0), AM, DeadNodes, Depth+1) && + if (!MatchAddressRecursively(N.getOperand(0), AM, Depth+1) && // Address could not have picked a GV address for the displacement. AM.GV == NULL && // On x86-64, the resultant disp must fit in 32-bits. @@ -1073,7 +1047,7 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, CurDAG->RepositionNode(N.getNode(), Shl.getNode()); Shl.getNode()->setNodeId(N.getNode()->getNodeId()); } - CurDAG->ReplaceAllUsesWith(N, Shl, &DeadNodes); + CurDAG->ReplaceAllUsesWith(N, Shl); AM.IndexReg = And; AM.Scale = (1 << ScaleLog); return false; @@ -1124,7 +1098,7 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM, NewSHIFT.getNode()->setNodeId(N.getNode()->getNodeId()); } - CurDAG->ReplaceAllUsesWith(N, NewSHIFT, &DeadNodes); + CurDAG->ReplaceAllUsesWith(N, NewSHIFT); AM.Scale = 1 << ShiftCst; AM.IndexReg = NewAND; diff --git a/test/CodeGen/X86/2010-03-17-ISelBug.ll b/test/CodeGen/X86/2010-03-17-ISelBug.ll index 609b4e24900..ba21902f7d0 100644 --- a/test/CodeGen/X86/2010-03-17-ISelBug.ll +++ b/test/CodeGen/X86/2010-03-17-ISelBug.ll @@ -1,4 +1,5 @@ ; RUN: llc < %s -mtriple=i386-apple-darwin5 + ; rdar://7761790 %"struct..0$_485" = type { i16, i16, i32 } @@ -37,3 +38,30 @@ bb169: ; preds = %bb169, %bb.nph380 %4 = add nsw i32 %index.6379, 1 ; [#uses=1] br label %bb169 } + +; PR7368 + +%struct.bufBit_s = type { i8*, i8 } + +define fastcc void @printSwipe([2 x [256 x %struct.bufBit_s]]* nocapture %colourLines) nounwind { +entry: + br label %for.body190 + +for.body261.i: ; preds = %for.body261.i, %for.body190 + %line.3300.i = phi i32 [ undef, %for.body190 ], [ %add292.i, %for.body261.i ] ; [#uses=3] + %conv268.i = and i32 %line.3300.i, 255 ; [#uses=1] + %tmp278.i = getelementptr [2 x [256 x %struct.bufBit_s]]* %colourLines, i32 undef, i32 %pen.1100, i32 %conv268.i, i32 0 ; [#uses=1] + store i8* undef, i8** %tmp278.i + %tmp338 = shl i32 %line.3300.i, 3 ; [#uses=1] + %tmp339 = and i32 %tmp338, 2040 ; [#uses=1] + %tmp285.i = getelementptr i8* %scevgep328, i32 %tmp339 ; [#uses=1] + store i8 undef, i8* %tmp285.i + %add292.i = add nsw i32 0, %line.3300.i ; [#uses=1] + br i1 undef, label %for.body190, label %for.body261.i + +for.body190: ; preds = %for.body261.i, %for.body190, %bb.nph104 + %pen.1100 = phi i32 [ 0, %entry ], [ %inc230, %for.body261.i ], [ %inc230, %for.body190 ] ; [#uses=3] + %scevgep328 = getelementptr [2 x [256 x %struct.bufBit_s]]* %colourLines, i32 undef, i32 %pen.1100, i32 0, i32 1 ; [#uses=1] + %inc230 = add i32 %pen.1100, 1 ; [#uses=2] + br i1 undef, label %for.body190, label %for.body261.i +}