Skip to content

Commit 7be1528

Browse files
author
Evan Cheng
committed
Fixes a nasty dag combiner bug that causes a bunch of tests to fail at -O0.
It's not safe to use the two value CombineTo variant to combine away a dead load. e.g. v1, chain2 = load chain1, loc v2, chain3 = load chain2, loc v3 = add v2, c Now we replace use of v1 with undef, use of chain2 with chain1. ReplaceAllUsesWith() will iterate through uses of the first load and update operands: v1, chain2 = load chain1, loc v2, chain3 = load chain1, loc v3 = add v2, c Now the second load is the same as the first load, SelectionDAG cse will ensure the use of second load is replaced with the first load. v1, chain2 = load chain1, loc v3 = add v1, c Then v1 is replaced with undef and bad things happen. llvm-svn: 46099
1 parent 32b0ff6 commit 7be1528

File tree

2 files changed

+74
-6
lines changed

2 files changed

+74
-6
lines changed

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4055,16 +4055,54 @@ SDOperand DAGCombiner::visitLOAD(SDNode *N) {
40554055
if (!LD->isVolatile()) {
40564056
if (N->getValueType(1) == MVT::Other) {
40574057
// Unindexed loads.
4058-
if (N->hasNUsesOfValue(0, 0))
4059-
return CombineTo(N, DAG.getNode(ISD::UNDEF, N->getValueType(0)), Chain);
4058+
if (N->hasNUsesOfValue(0, 0)) {
4059+
// It's not safe to use the two value CombineTo variant here. e.g.
4060+
// v1, chain2 = load chain1, loc
4061+
// v2, chain3 = load chain2, loc
4062+
// v3 = add v2, c
4063+
// Now we replace use of v1 with undef, use of chain2 with chain1.
4064+
// ReplaceAllUsesWith() will iterate through uses of the first load and
4065+
// update operands:
4066+
// v1, chain2 = load chain1, loc
4067+
// v2, chain3 = load chain1, loc
4068+
// v3 = add v2, c
4069+
// Now the second load is the same as the first load, SelectionDAG cse
4070+
// will ensure the use of second load is replaced with the first load.
4071+
// v1, chain2 = load chain1, loc
4072+
// v3 = add v1, c
4073+
// Then v1 is replaced with undef and bad things happen.
4074+
std::vector<SDNode*> NowDead;
4075+
SDOperand Undef = DAG.getNode(ISD::UNDEF, N->getValueType(0));
4076+
DOUT << "\nReplacing.6 "; DEBUG(N->dump(&DAG));
4077+
DOUT << "\nWith: "; DEBUG(Undef.Val->dump(&DAG));
4078+
DOUT << " and 1 other value\n";
4079+
DAG.ReplaceAllUsesOfValueWith(SDOperand(N, 0), Undef, &NowDead);
4080+
DAG.ReplaceAllUsesOfValueWith(SDOperand(N, 1), Chain, &NowDead);
4081+
removeFromWorkList(N);
4082+
for (unsigned i = 0, e = NowDead.size(); i != e; ++i)
4083+
removeFromWorkList(NowDead[i]);
4084+
DAG.DeleteNode(N);
4085+
return SDOperand(N, 0); // Return N so it doesn't get rechecked!
4086+
}
40604087
} else {
40614088
// Indexed loads.
40624089
assert(N->getValueType(2) == MVT::Other && "Malformed indexed loads?");
40634090
if (N->hasNUsesOfValue(0, 0) && N->hasNUsesOfValue(0, 1)) {
4064-
SDOperand Undef0 = DAG.getNode(ISD::UNDEF, N->getValueType(0));
4065-
SDOperand Undef1 = DAG.getNode(ISD::UNDEF, N->getValueType(1));
4066-
SDOperand To[] = { Undef0, Undef1, Chain };
4067-
return CombineTo(N, To, 3);
4091+
std::vector<SDNode*> NowDead;
4092+
SDOperand Undef = DAG.getNode(ISD::UNDEF, N->getValueType(0));
4093+
DOUT << "\nReplacing.6 "; DEBUG(N->dump(&DAG));
4094+
DOUT << "\nWith: "; DEBUG(Undef.Val->dump(&DAG));
4095+
DOUT << " and 2 other values\n";
4096+
DAG.ReplaceAllUsesOfValueWith(SDOperand(N, 0), Undef, &NowDead);
4097+
DAG.ReplaceAllUsesOfValueWith(SDOperand(N, 1),
4098+
DAG.getNode(ISD::UNDEF, N->getValueType(1)),
4099+
&NowDead);
4100+
DAG.ReplaceAllUsesOfValueWith(SDOperand(N, 2), Chain, &NowDead);
4101+
removeFromWorkList(N);
4102+
for (unsigned i = 0, e = NowDead.size(); i != e; ++i)
4103+
removeFromWorkList(NowDead[i]);
4104+
DAG.DeleteNode(N);
4105+
return SDOperand(N, 0); // Return N so it doesn't get rechecked!
40684106
}
40694107
}
40704108
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
; RUN: llvm-as < %s | llc -march=x86 | not grep IMPLICIT_DEF
2+
3+
%struct.node_t = type { double*, %struct.node_t*, %struct.node_t**, double**, double*, i32, i32 }
4+
5+
define void @localize_local_bb19_bb(%struct.node_t** %cur_node) {
6+
newFuncRoot:
7+
%tmp1 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
8+
%tmp2 = getelementptr %struct.node_t* %tmp1, i32 0, i32 4 ; <double**> [#uses=1]
9+
%tmp3 = load double** %tmp2, align 4 ; <double*> [#uses=1]
10+
%tmp4 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
11+
%tmp5 = getelementptr %struct.node_t* %tmp4, i32 0, i32 4 ; <double**> [#uses=1]
12+
store double* %tmp3, double** %tmp5, align 4
13+
%tmp6 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
14+
%tmp7 = getelementptr %struct.node_t* %tmp6, i32 0, i32 3 ; <double***> [#uses=1]
15+
%tmp8 = load double*** %tmp7, align 4 ; <double**> [#uses=1]
16+
%tmp9 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
17+
%tmp10 = getelementptr %struct.node_t* %tmp9, i32 0, i32 3 ; <double***> [#uses=1]
18+
store double** %tmp8, double*** %tmp10, align 4
19+
%tmp11 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
20+
%tmp12 = getelementptr %struct.node_t* %tmp11, i32 0, i32 0 ; <double**> [#uses=1]
21+
%tmp13 = load double** %tmp12, align 4 ; <double*> [#uses=1]
22+
%tmp14 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
23+
%tmp15 = getelementptr %struct.node_t* %tmp14, i32 0, i32 0 ; <double**> [#uses=1]
24+
store double* %tmp13, double** %tmp15, align 4
25+
%tmp16 = load %struct.node_t** %cur_node, align 4 ; <%struct.node_t*> [#uses=1]
26+
%tmp17 = getelementptr %struct.node_t* %tmp16, i32 0, i32 1 ; <%struct.node_t**> [#uses=1]
27+
%tmp18 = load %struct.node_t** %tmp17, align 4 ; <%struct.node_t*> [#uses=1]
28+
store %struct.node_t* %tmp18, %struct.node_t** %cur_node, align 4
29+
ret void
30+
}

0 commit comments

Comments
 (0)