This is an archive of the discontinued LLVM Phabricator instance.

Differential D65490

[SimplifyCFG] Mark missed Changed to true.
ClosedPublic

Authored by asbirlea on Jul 30 2019, 4:59 PM.

Details

Reviewers
zhizhouy
manojgupta
sanjoy
Commits
rG3af2a69575da: [SimplifyCFG] Mark missed Changed to true.
rL367596: [SimplifyCFG] Mark missed Changed to true.
Summary

DominatorTree is invalid after SimplifyCFG because of a missed Changed = true when simplifying a branch condition and removing an edge.
Resolves PR42272.

Diff Detail

Event Timeline

asbirlea created this revision.Jul 30 2019, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 4:59 PM
Herald added subscribers: sanjoy.google, jlebar. · View Herald Transcript
Harbormaster completed remote builds in B35853: Diff 212473.Jul 30 2019, 5:01 PM
leonardchan added a subscriber: leonardchan.Jul 31 2019, 5:46 PM

*ping* We would like to bump up the priority of this since this allows us to enable the new PM when building fuchsia.

sanjoy accepted this revision.Jul 31 2019, 5:57 PM
sanjoy added a subscriber: sanjoy.

lgtm

lib/Transforms/Utils/SimplifyCFG.cpp
4181

Is this check necessary? If not, might be nice to remove it (in a separate change).

4192

You could sink down the Changed = true; outside the if/else.

test/Transforms/SimplifyCFG/invalidate-dom.ll
17

You can use opt -metarenamer to give names to all these instructions. That makes the test a bit more resilient to changes.

This revision is now accepted and ready to land.Jul 31 2019, 5:57 PM
asbirlea updated this revision to Diff 212863.Aug 1 2019, 11:28 AM
asbirlea marked 4 inline comments as done.

Address comments.

lib/Transforms/Utils/SimplifyCFG.cpp
4192

Right, but this is non-obvious because of the else if below (instead of else). Unless one notices that this iterates over BB's predecessors, then they may think BB may not be either the 0 or 1 successor.
Let me send the clean-up patch for the above and for this separately, since we can also sink the call to EraseTerminatorAndDCECond(BI); here.

test/Transforms/SimplifyCFG/invalidate-dom.ll
17

SG, but I renamed just the instructions manually. The metarenamer renames everything and I for one get more confused when everything is named foo, bar etc :).

Harbormaster completed remote builds in B35978: Diff 212863.Aug 1 2019, 11:30 AM
Closed by commit rL367596: [SimplifyCFG] Mark missed Changed to true. (authored by asbirlea). · Explain WhyAug 1 2019, 11:37 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Transforms/
Utils/
1 line
test/
Transforms/
SimplifyCFG/
90 lines

Diff 212863

lib/Transforms/Utils/SimplifyCFG.cpp

Show First 20 Lines • Show All 4,172 Lines • ▼ Show 20 Lines if (&BB->front() != UI)
return Changed; return Changed;
SmallVector<BasicBlock *, 8> Preds(pred_begin(BB), pred_end(BB)); SmallVector<BasicBlock *, 8> Preds(pred_begin(BB), pred_end(BB));
for (unsigned i = 0, e = Preds.size(); i != e; ++i) { for (unsigned i = 0, e = Preds.size(); i != e; ++i) {
Instruction *TI = Preds[i]->getTerminator(); Instruction *TI = Preds[i]->getTerminator();
IRBuilder<> Builder(TI); IRBuilder<> Builder(TI);
if (auto *BI = dyn_cast<BranchInst>(TI)) { if (auto *BI = dyn_cast<BranchInst>(TI)) {
if (BI->isUnconditional()) { if (BI->isUnconditional()) {
if (BI->getSuccessor(0) == BB) { if (BI->getSuccessor(0) == BB) {
sanjoyUnsubmitted
Done
ReplyInline Actions

Is this check necessary? If not, might be nice to remove it (in a separate change).

sanjoy: Is this check necessary? If not, might be nice to remove it (in a separate change).
new UnreachableInst(TI->getContext(), TI); new UnreachableInst(TI->getContext(), TI);
TI->eraseFromParent(); TI->eraseFromParent();
Changed = true; Changed = true;
} }
} else { } else {
Value* Cond = BI->getCondition(); Value* Cond = BI->getCondition();
if (BI->getSuccessor(0) == BB) { if (BI->getSuccessor(0) == BB) {
Builder.CreateAssumption(Builder.CreateNot(Cond)); Builder.CreateAssumption(Builder.CreateNot(Cond));
Builder.CreateBr(BI->getSuccessor(1)); Builder.CreateBr(BI->getSuccessor(1));
EraseTerminatorAndDCECond(BI); EraseTerminatorAndDCECond(BI);
Changed = true;
sanjoyUnsubmitted
Done
ReplyInline Actions

You could sink down the Changed = true; outside the if/else.

sanjoy: You could sink down the `Changed = true;` outside the if/else.
asbirleaAuthorUnsubmitted
Not Done
ReplyInline Actions

Right, but this is non-obvious because of the else if below (instead of else). Unless one notices that this iterates over BB's predecessors, then they may think BB may not be either the 0 or 1 successor.
Let me send the clean-up patch for the above and for this separately, since we can also sink the call to EraseTerminatorAndDCECond(BI); here.

asbirlea: Right, but this is non-obvious because of the `else if` below (instead of `else`). Unless one…
} else if (BI->getSuccessor(1) == BB) { } else if (BI->getSuccessor(1) == BB) {
Builder.CreateAssumption(Cond); Builder.CreateAssumption(Cond);
Builder.CreateBr(BI->getSuccessor(0)); Builder.CreateBr(BI->getSuccessor(0));
EraseTerminatorAndDCECond(BI); EraseTerminatorAndDCECond(BI);
Changed = true; Changed = true;
} }
} }
} else if (auto *SI = dyn_cast<SwitchInst>(TI)) { } else if (auto *SI = dyn_cast<SwitchInst>(TI)) {
▲ Show 20 Lines • Show All 1,892 Lines • Show Last 20 Lines

test/Transforms/SimplifyCFG/invalidate-dom.ll

  • This file was added.
; RUN: opt -aa-pipeline=default -passes="default<O2>" -disable-output %s
; REQUIRES: asserts
; PR42272
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
@b = external global i32, align 4
@f = external global i32, align 4
@g = external global i32, align 4
define i32* @a(i32 %h) #0 {
entry:
%h.addr = alloca i32, align 4
store i32 %h, i32* %h.addr, align 4
%tmp0 = load i32, i32* %h.addr, align 4
switch i32 %tmp0, label %sw.default [
sanjoyUnsubmitted
Done
ReplyInline Actions

You can use opt -metarenamer to give names to all these instructions. That makes the test a bit more resilient to changes.

sanjoy: You can use `opt -metarenamer` to give names to all these instructions. That makes the test a…
asbirleaAuthorUnsubmitted
Done
ReplyInline Actions

SG, but I renamed just the instructions manually. The metarenamer renames everything and I for one get more confused when everything is named foo, bar etc :).

asbirlea: SG, but I renamed just the instructions manually. The metarenamer renames everything and I for…
i32 4, label %sw.bb
i32 3, label %sw.bb1
i32 2, label %sw.bb3
]
sw.bb: ; preds = %entry
%call = call i32 (...) @c()
unreachable
sw.bb1: ; preds = %entry
%call2 = call i32 (...) @c()
unreachable
sw.bb3: ; preds = %entry
%call4 = call i32 (...) @c()
%conv = sext i32 %call4 to i64
%tmp1 = inttoptr i64 %conv to i32*
ret i32* %tmp1
sw.default: ; preds = %entry
unreachable
}
define i32 @m() #1 {
entry:
%call = call i32 @j()
%call1 = call i32 @j()
ret i32 undef
}
define internal i32 @j() #0 {
entry:
%i = alloca i32, align 4
store i32 0, i32* %i, align 4
br label %for.cond
for.cond: ; preds = %for.body, %entry
%tmp0 = load i32, i32* %i, align 4
%tmp1 = load i32, i32* @f, align 4
%cmp = icmp ult i32 %tmp0, %tmp1
br i1 %cmp, label %for.body, label %for.cond.cleanup
for.cond.cleanup: ; preds = %for.cond
%tmp2 = load i32, i32* @f, align 4
%call3 = call i32* @a(i32 %tmp2)
ret i32 undef
for.body: ; preds = %for.cond
%call = call i32 (...) @c()
%call1 = call i32 (...) @c()
%call2 = call i32 (...) @c()
%tmp3 = load i32, i32* @b, align 4
%tmp4 = load i32, i32* @g, align 4
%sub = sub nsw i32 %tmp4, %tmp3
store i32 %sub, i32* @g, align 4
%tmp5 = load i32, i32* %i, align 4
%inc = add i32 %tmp5, 1
store i32 %inc, i32* %i, align 4
br label %for.cond
}
declare i32 @c(...) #0
; Function Attrs: argmemonly nounwind
declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) #2
; Function Attrs: argmemonly nounwind
declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture) #2
attributes #0 = { "use-soft-float"="false" }
attributes #1 = { "target-cpu"="x86-64" }
attributes #2 = { argmemonly nounwind }