This is an archive of the discontinued LLVM Phabricator instance.

Differential D31000

AMDGPU: Fix handling of constant phi input loop conditions
ClosedPublic

Authored by arsenm on Mar 15 2017, 1:59 PM.

Details

Reviewers
cfang
tstellar
Summary

If the loop condition was an i1 phi with a constantexpr input, this
would add a loop intrinsic fed by a phi dependent on a call to
if.break in the same block. Insert the call in the loop header.

Diff Detail

Event Timeline

arsenm created this revision.Mar 15 2017, 1:59 PM
Herald added subscribers: tpr, dstuttard, tony-tye and 4 others. · View Herald TranscriptMar 15 2017, 1:59 PM
cfang added inline comments.Mar 15 2017, 2:24 PM
lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
298

Do you think for all constant "Cond" cases, the if.break intrinsics should be inserted to loop header? Not just Cond==BoolTrue?

Initially, we insert in the TERM just to resolve an unhandled assert!

arsenm added inline comments.Mar 15 2017, 2:29 PM
lib/Target/AMDGPU/SIAnnotateControlFlow.cpp
298

It's the opposite. BoolTrue is not inserted into header. Break true is the normal case. The loop before inserts a break rather than an if.break for this case.

I think the way this works is the assumption is the default incoming value is true. phi.broken is filled with 0s for the other predecessors, which with lowering to and_n2 exec, conditionmask is the same as branch true.

cfang accepted this revision.Mar 15 2017, 2:37 PM

LGTM It would be great if you can update the comment:
// Insert IfBreak before TERM for constant COND.

This revision is now accepted and ready to land.Mar 15 2017, 2:37 PM
arsenm closed this revision.Mar 17 2017, 2:04 PM

r298121

Revision Contents

PathSize
lib/
Target/
AMDGPU/
11 lines
test/
CodeGen/
AMDGPU/
258 lines

Diff 91930

lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

Show First 20 Lines • Show All 214 Lines • ▼ Show 20 LinesValue *SIAnnotateControlFlow::handleLoopCondition(Value *Cond, PHINode *Broken,
// with PHI nodes that are outside of the loop, we end up inserting new PHI // with PHI nodes that are outside of the loop, we end up inserting new PHI
// nodes outside of the loop which depend on values defined inside the loop. // nodes outside of the loop which depend on values defined inside the loop.
// This will break the module with // This will break the module with
// 'Instruction does not dominate all users!' errors. // 'Instruction does not dominate all users!' errors.
PHINode *Phi = nullptr; PHINode *Phi = nullptr;
if ((Phi = dyn_cast<PHINode>(Cond)) && L->contains(Phi)) { if ((Phi = dyn_cast<PHINode>(Cond)) && L->contains(Phi)) {
BasicBlock *Parent = Phi->getParent(); BasicBlock *Parent = Phi->getParent();
PHINode *NewPhi = PHINode::Create(Int64, 0, "", &Parent->front()); PHINode *NewPhi = PHINode::Create(Int64, 0, "loop.phi", &Parent->front());
Value *Ret = NewPhi; Value *Ret = NewPhi;
// Handle all non-constant incoming values first // Handle all non-constant incoming values first
for (unsigned i = 0, e = Phi->getNumIncomingValues(); i != e; ++i) { for (unsigned i = 0, e = Phi->getNumIncomingValues(); i != e; ++i) {
Value *Incoming = Phi->getIncomingValue(i); Value *Incoming = Phi->getIncomingValue(i);
BasicBlock *From = Phi->getIncomingBlock(i); BasicBlock *From = Phi->getIncomingBlock(i);
if (isa<ConstantInt>(Incoming)) { if (isa<ConstantInt>(Incoming)) {
NewPhi->addIncoming(Broken, From); NewPhi->addIncoming(Broken, From);
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Lines if (L->contains(Inst)) {
Insert = L->getHeader()->getFirstNonPHIOrDbgOrLifetime(); Insert = L->getHeader()->getFirstNonPHIOrDbgOrLifetime();
} }
Value *Args[] = { Cond, Broken }; Value *Args[] = { Cond, Broken };
return CallInst::Create(IfBreak, Args, "", Insert); return CallInst::Create(IfBreak, Args, "", Insert);
} }
// Insert IfBreak before TERM for constant COND. // Insert IfBreak before TERM for constant COND.
if (isa<ConstantInt>(Cond)) { if (isa<Constant>(Cond)) {
Instruction *Insert = Cond == BoolTrue ?
cfangUnsubmitted
Not Done
ReplyInline Actions

Do you think for all constant "Cond" cases, the if.break intrinsics should be inserted to loop header? Not just Cond==BoolTrue?

Initially, we insert in the TERM just to resolve an unhandled assert!

cfang: Do you think for all constant "Cond" cases, the if.break intrinsics should be inserted to loop…
arsenmAuthorUnsubmitted
Not Done
ReplyInline Actions

It's the opposite. BoolTrue is not inserted into header. Break true is the normal case. The loop before inserts a break rather than an if.break for this case.

I think the way this works is the assumption is the default incoming value is true. phi.broken is filled with 0s for the other predecessors, which with lowering to and_n2 exec, conditionmask is the same as branch true.

arsenm: It's the opposite. BoolTrue is not inserted into header. Break true is the normal case. The…
Term : L->getHeader()->getTerminator();
Value *Args[] = { Cond, Broken }; Value *Args[] = { Cond, Broken };
return CallInst::Create(IfBreak, Args, "", Term); return CallInst::Create(IfBreak, Args, "", Insert);
} }
llvm_unreachable("Unhandled loop condition!"); llvm_unreachable("Unhandled loop condition!");
} }
/// \brief Handle a back edge (loop) /// \brief Handle a back edge (loop)
void SIAnnotateControlFlow::handleLoop(BranchInst *Term) { void SIAnnotateControlFlow::handleLoop(BranchInst *Term) {
if (isUniform(Term)) if (isUniform(Term))
return; return;
BasicBlock *BB = Term->getParent(); BasicBlock *BB = Term->getParent();
llvm::Loop *L = LI->getLoopFor(BB); llvm::Loop *L = LI->getLoopFor(BB);
if (!L) if (!L)
return; return;
BasicBlock *Target = Term->getSuccessor(1); BasicBlock *Target = Term->getSuccessor(1);
PHINode *Broken = PHINode::Create(Int64, 0, "", &Target->front()); PHINode *Broken = PHINode::Create(Int64, 0, "phi.broken", &Target->front());
Value *Cond = Term->getCondition(); Value *Cond = Term->getCondition();
Term->setCondition(BoolTrue); Term->setCondition(BoolTrue);
Value *Arg = handleLoopCondition(Cond, Broken, L, Term); Value *Arg = handleLoopCondition(Cond, Broken, L, Term);
for (BasicBlock *Pred : predecessors(Target)) for (BasicBlock *Pred : predecessors(Target))
Broken->addIncoming(Pred == BB ? Arg : Int64Zero, Pred); Broken->addIncoming(Pred == BB ? Arg : Int64Zero, Pred);
▲ Show 20 Lines • Show All 81 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/loop_break.ll

Show First 20 Lines • Show All 58 Lines • ▼ Show 20 Linesbb4:
%load = load volatile i32, i32 addrspace(1)* undef, align 4 %load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp slt i32 %tmp, %load %cmp1 = icmp slt i32 %tmp, %load
br i1 %cmp1, label %bb1, label %bb9 br i1 %cmp1, label %bb1, label %bb9
bb9: bb9:
ret void ret void
} }
; OPT-LABEL: @undef_phi_cond_break_loop(
; OPT: bb1:
; OPT-NEXT: %phi.broken = phi i64 [ %loop.phi, %Flow ], [ 0, %bb ]
; OPT-NEXT: %lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
; OPT: %0 = call i64 @llvm.amdgcn.if.break(i1 undef, i64 %phi.broken)
; OPT-NEXT: br i1 %cmp0, label %bb4, label %Flow
; OPT: bb4:
; OPT-NEXT: %load = load volatile i32, i32 addrspace(1)* undef, align 4
; OPT-NEXT: %cmp1 = icmp sge i32 %tmp, %load
; OPT-NEXT: %1 = call i64 @llvm.amdgcn.if.break(i1 %cmp1, i64 %phi.broken)
; OPT-NEXT: br label %Flow
; OPT: Flow:
; OPT-NEXT: %loop.phi = phi i64 [ %1, %bb4 ], [ %0, %bb1 ]
; OPT-NEXT: %tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
; OPT-NEXT: %2 = call i1 @llvm.amdgcn.loop(i64 %loop.phi)
; OPT-NEXT: br i1 %2, label %bb9, label %bb1
; OPT: bb9: ; preds = %Flow
; OPT-NEXT: call void @llvm.amdgcn.end.cf(i64 %loop.phi)
; OPT-NEXT: store volatile i32 7
; OPT-NEXT: ret void
define void @undef_phi_cond_break_loop(i32 %arg) #0 {
bb:
%id = call i32 @llvm.amdgcn.workitem.id.x()
%tmp = sub i32 %id, %arg
br label %bb1
bb1: ; preds = %Flow, %bb
%lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
%lsr.iv.next = add i32 %lsr.iv, 1
%cmp0 = icmp slt i32 %lsr.iv.next, 0
br i1 %cmp0, label %bb4, label %Flow
bb4: ; preds = %bb1
%load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp sge i32 %tmp, %load
br label %Flow
Flow: ; preds = %bb4, %bb1
%tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
%tmp3 = phi i1 [ %cmp1, %bb4 ], [ undef, %bb1 ]
br i1 %tmp3, label %bb9, label %bb1
bb9: ; preds = %Flow
store volatile i32 7, i32 addrspace(3)* undef
ret void
}
; FIXME: ConstantExpr compare of address to null folds away
@lds = addrspace(3) global i32 undef
; OPT-LABEL: @constexpr_phi_cond_break_loop(
; OPT: bb1:
; OPT-NEXT: %phi.broken = phi i64 [ %loop.phi, %Flow ], [ 0, %bb ]
; OPT-NEXT: %lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
; OPT: %0 = call i64 @llvm.amdgcn.if.break(i1 icmp ne (i32 addrspace(3)* inttoptr (i32 4 to i32 addrspace(3)*), i32 addrspace(3)* @lds), i64 %phi.broken)
; OPT-NEXT: br i1 %cmp0, label %bb4, label %Flow
; OPT: bb4:
; OPT-NEXT: %load = load volatile i32, i32 addrspace(1)* undef, align 4
; OPT-NEXT: %cmp1 = icmp sge i32 %tmp, %load
; OPT-NEXT: %1 = call i64 @llvm.amdgcn.if.break(i1 %cmp1, i64 %phi.broken)
; OPT-NEXT: br label %Flow
; OPT: Flow:
; OPT-NEXT: %loop.phi = phi i64 [ %1, %bb4 ], [ %0, %bb1 ]
; OPT-NEXT: %tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
; OPT-NEXT: %2 = call i1 @llvm.amdgcn.loop(i64 %loop.phi)
; OPT-NEXT: br i1 %2, label %bb9, label %bb1
; OPT: bb9: ; preds = %Flow
; OPT-NEXT: call void @llvm.amdgcn.end.cf(i64 %loop.phi)
; OPT-NEXT: store volatile i32 7
; OPT-NEXT: ret void
define void @constexpr_phi_cond_break_loop(i32 %arg) #0 {
bb:
%id = call i32 @llvm.amdgcn.workitem.id.x()
%tmp = sub i32 %id, %arg
br label %bb1
bb1: ; preds = %Flow, %bb
%lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
%lsr.iv.next = add i32 %lsr.iv, 1
%cmp0 = icmp slt i32 %lsr.iv.next, 0
br i1 %cmp0, label %bb4, label %Flow
bb4: ; preds = %bb1
%load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp sge i32 %tmp, %load
br label %Flow
Flow: ; preds = %bb4, %bb1
%tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
%tmp3 = phi i1 [ %cmp1, %bb4 ], [ icmp ne (i32 addrspace(3)* inttoptr (i32 4 to i32 addrspace(3)*), i32 addrspace(3)* @lds), %bb1 ]
br i1 %tmp3, label %bb9, label %bb1
bb9: ; preds = %Flow
store volatile i32 7, i32 addrspace(3)* undef
ret void
}
; OPT-LABEL: @true_phi_cond_break_loop(
; OPT: bb1:
; OPT-NEXT: %phi.broken = phi i64 [ %loop.phi, %Flow ], [ 0, %bb ]
; OPT-NEXT: %lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
; OPT: %0 = call i64 @llvm.amdgcn.break(i64 %phi.broken)
; OPT: br i1 %cmp0, label %bb4, label %Flow
; OPT: bb4:
; OPT-NEXT: %load = load volatile i32, i32 addrspace(1)* undef, align 4
; OPT-NEXT: %cmp1 = icmp sge i32 %tmp, %load
; OPT-NEXT: %1 = call i64 @llvm.amdgcn.if.break(i1 %cmp1, i64 %phi.broken)
; OPT-NEXT: br label %Flow
; OPT: Flow:
; OPT-NEXT: %loop.phi = phi i64 [ %1, %bb4 ], [ %0, %bb1 ]
; OPT-NEXT: %tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
; OPT-NEXT: %2 = call i1 @llvm.amdgcn.loop(i64 %loop.phi)
; OPT-NEXT: br i1 %2, label %bb9, label %bb1
; OPT: bb9: ; preds = %Flow
; OPT-NEXT: call void @llvm.amdgcn.end.cf(i64 %loop.phi)
; OPT-NEXT: store volatile i32 7
; OPT-NEXT: ret void
define void @true_phi_cond_break_loop(i32 %arg) #0 {
bb:
%id = call i32 @llvm.amdgcn.workitem.id.x()
%tmp = sub i32 %id, %arg
br label %bb1
bb1: ; preds = %Flow, %bb
%lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
%lsr.iv.next = add i32 %lsr.iv, 1
%cmp0 = icmp slt i32 %lsr.iv.next, 0
br i1 %cmp0, label %bb4, label %Flow
bb4: ; preds = %bb1
%load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp sge i32 %tmp, %load
br label %Flow
Flow: ; preds = %bb4, %bb1
%tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
%tmp3 = phi i1 [ %cmp1, %bb4 ], [ true, %bb1 ]
br i1 %tmp3, label %bb9, label %bb1
bb9: ; preds = %Flow
store volatile i32 7, i32 addrspace(3)* undef
ret void
}
; OPT-LABEL: @false_phi_cond_break_loop(
; OPT: bb1:
; OPT-NEXT: %phi.broken = phi i64 [ %loop.phi, %Flow ], [ 0, %bb ]
; OPT-NEXT: %lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
; OPT-NOT: call
; OPT: br i1 %cmp0, label %bb4, label %Flow
; OPT: bb4:
; OPT-NEXT: %load = load volatile i32, i32 addrspace(1)* undef, align 4
; OPT-NEXT: %cmp1 = icmp sge i32 %tmp, %load
; OPT-NEXT: %0 = call i64 @llvm.amdgcn.if.break(i1 %cmp1, i64 %phi.broken)
; OPT-NEXT: br label %Flow
; OPT: Flow:
; OPT-NEXT: %loop.phi = phi i64 [ %0, %bb4 ], [ %phi.broken, %bb1 ]
; OPT-NEXT: %tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
; OPT-NEXT: %1 = call i1 @llvm.amdgcn.loop(i64 %loop.phi)
; OPT-NEXT: br i1 %1, label %bb9, label %bb1
; OPT: bb9: ; preds = %Flow
; OPT-NEXT: call void @llvm.amdgcn.end.cf(i64 %loop.phi)
; OPT-NEXT: store volatile i32 7
; OPT-NEXT: ret void
define void @false_phi_cond_break_loop(i32 %arg) #0 {
bb:
%id = call i32 @llvm.amdgcn.workitem.id.x()
%tmp = sub i32 %id, %arg
br label %bb1
bb1: ; preds = %Flow, %bb
%lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
%lsr.iv.next = add i32 %lsr.iv, 1
%cmp0 = icmp slt i32 %lsr.iv.next, 0
br i1 %cmp0, label %bb4, label %Flow
bb4: ; preds = %bb1
%load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp sge i32 %tmp, %load
br label %Flow
Flow: ; preds = %bb4, %bb1
%tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
%tmp3 = phi i1 [ %cmp1, %bb4 ], [ false, %bb1 ]
br i1 %tmp3, label %bb9, label %bb1
bb9: ; preds = %Flow
store volatile i32 7, i32 addrspace(3)* undef
ret void
}
; Swap order of branches in flow block so that the true phi is
; continue.
; OPT-LABEL: @invert_true_phi_cond_break_loop(
; OPT: bb1:
; OPT-NEXT: %phi.broken = phi i64 [ %1, %Flow ], [ 0, %bb ]
; OPT-NEXT: %lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
; OPT-NEXT: %lsr.iv.next = add i32 %lsr.iv, 1
; OPT-NEXT: %cmp0 = icmp slt i32 %lsr.iv.next, 0
; OPT-NEXT: br i1 %cmp0, label %bb4, label %Flow
; OPT: bb4:
; OPT-NEXT: %load = load volatile i32, i32 addrspace(1)* undef, align 4
; OPT-NEXT: %cmp1 = icmp sge i32 %tmp, %load
; OPT-NEXT: br label %Flow
; OPT: Flow:
; OPT-NEXT: %tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
; OPT-NEXT: %tmp3 = phi i1 [ %cmp1, %bb4 ], [ true, %bb1 ]
; OPT-NEXT: %0 = xor i1 %tmp3, true
; OPT-NEXT: %1 = call i64 @llvm.amdgcn.if.break(i1 %0, i64 %phi.broken)
; OPT-NEXT: %2 = call i1 @llvm.amdgcn.loop(i64 %1)
; OPT-NEXT: br i1 %2, label %bb9, label %bb1
; OPT: bb9:
; OPT-NEXT: call void @llvm.amdgcn.end.cf(i64 %1)
; OPT-NEXT: store volatile i32 7, i32 addrspace(3)* undef
; OPT-NEXT: ret void
define void @invert_true_phi_cond_break_loop(i32 %arg) #0 {
bb:
%id = call i32 @llvm.amdgcn.workitem.id.x()
%tmp = sub i32 %id, %arg
br label %bb1
bb1: ; preds = %Flow, %bb
%lsr.iv = phi i32 [ undef, %bb ], [ %tmp2, %Flow ]
%lsr.iv.next = add i32 %lsr.iv, 1
%cmp0 = icmp slt i32 %lsr.iv.next, 0
br i1 %cmp0, label %bb4, label %Flow
bb4: ; preds = %bb1
%load = load volatile i32, i32 addrspace(1)* undef, align 4
%cmp1 = icmp sge i32 %tmp, %load
br label %Flow
Flow: ; preds = %bb4, %bb1
%tmp2 = phi i32 [ %lsr.iv.next, %bb4 ], [ undef, %bb1 ]
%tmp3 = phi i1 [ %cmp1, %bb4 ], [ true, %bb1 ]
br i1 %tmp3, label %bb1, label %bb9
bb9: ; preds = %Flow
store volatile i32 7, i32 addrspace(3)* undef
ret void
}
declare i32 @llvm.amdgcn.workitem.id.x() #1 declare i32 @llvm.amdgcn.workitem.id.x() #1
attributes #0 = { nounwind } attributes #0 = { nounwind }
attributes #1 = { nounwind readnone } attributes #1 = { nounwind readnone }