[llvm] r292944 - [SelectionDAG] Handle inverted conditions when splitting into multiple branches.
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 02:41:05 PST 2017
Hi Geoff,
I found a problem with this commit on my out-of-tree target, but it
reproduces on xcore as well with
llc -O0 -march=xcore -o - foo.ll -verify-machineinstrs
I have this input:
@a = external local_unnamed_addr global i16, align 1
; Function Attrs: norecurse nounwind
define void @f2() {
entry:
%0 = load i16, i16* @a, align 1
%cmp = icmp sgt i16 %0, 0
br label %lor.rhs.i
lor.rhs.i: ; preds = %entry
%cmp.i = xor i1 %cmp, true
%or.cond1.i = and i1 %cmp.i, icmp ne (i16 ptrtoint (void () * @f2 to
i16), i16 0)
br i1 %or.cond1.i, label %f1.exit, label %f1.exit
f1.exit: ; preds = %lor.rhs.i
ret void
}
The FindMergedConditions stuff triggers on the and:
%or.cond1.i = and i1 %cmp.i, icmp ne (i16 ptrtoint (void () * @f2 to
i16), i16 0)
and %cmp.i is examined.
%cmp.i = xor i1 %cmp, true
is identified as a "isNot" by your new code
> + // Skip over not part of the tree and remember to invert op and
operands at
> + // next level.
> + if (BinaryOperator::isNot(Cond) && Cond->hasOneUse()) {
> + Cond = cast<Instruction>(Cond)->getOperand(0);
> + FindMergedConditions(Cond, TBB, FBB, CurBB, SwitchBB, Opc,
TProb, FProb,
> + !InvertCond);
> + return;
> + }
and here it gets interesting because
Cond is now set to
%cmp = icmp sgt i16 %0, 0
which is in fact in a different basic block than the and.
So Cond and specifically Cond->getOperand(0) is not in CurBB.
This later leads to the verifier complaining about an undefined vreg
being used:
# After Instruction Selection
# Machine code for function f2: IsSSA, TracksLiveness
BB#0: derived from LLVM BB %entry
%vreg1<def> = LDAWDP_lru6 <ga:@a>; GRRegs:%vreg1
%vreg2<def> = LDC_ru6 0; GRRegs:%vreg2
%vreg3<def> = LD8U_3r %vreg1, %vreg2;
mem:LD1[@a](dereferenceable) GRRegs:%vreg3,%vreg1,%vreg2
%vreg4<def> = MKMSK_rus 1; GRRegs:%vreg4
%vreg5<def> = LD8U_3r %vreg1, %vreg4<kill>;
mem:LD1[@a+1](dereferenceable) GRRegs:%vreg5,%vreg1,%vreg4
%vreg6<def,tied1> = SEXT_rus %vreg5<tied0>, 8; GRRegs:%vreg6,%vreg5
%vreg7<def> = SHL_2rus %vreg6<kill>, 8; GRRegs:%vreg7,%vreg6
%vreg8<def> = OR_3r %vreg7<kill>, %vreg3<kill>;
GRRegs:%vreg8,%vreg7,%vreg3
%vreg0<def> = LSS_3r %vreg2, %vreg8<kill>;
GRRegs:%vreg0,%vreg2,%vreg8
BRFU_lu6 <BB#1>
Successors according to CFG: BB#1(?%)
BB#1: derived from LLVM BB %lor.rhs.i
Predecessors according to CFG: BB#0
%vreg10<def,tied1> = SEXT_rus %vreg9<tied0>, 16;
GRRegs:%vreg10,%vreg9
%vreg11<def> = LDC_ru6 0; GRRegs:%vreg11
%vreg12<def> = LSS_3r %vreg11<kill>, %vreg10<kill>;
GRRegs:%vreg12,%vreg11,%vreg10
BRFT_lru6 %vreg12<kill>, <BB#2>; GRRegs:%vreg12
BRFU_lu6 <BB#3>
Successors according to CFG: BB#3 BB#2
BB#3: derived from LLVM BB %lor.rhs.i
Predecessors according to CFG: BB#1
LDAPF_lu10 <ga:@f2>, %R11<imp-def>
%vreg13<def> = COPY %R11; GRRegs:%vreg13
%vreg14<def,tied1> = ZEXT_rus %vreg13<tied0>, 16;
GRRegs:%vreg14,%vreg13
BRFF_lru6 %vreg14<kill>, <BB#2>; GRRegs:%vreg14
BRFU_lu6 <BB#2>
Successors according to CFG: BB#2
BB#2: derived from LLVM BB %f1.exit
Predecessors according to CFG: BB#1 BB#3
RETSP_u6 0, %SP<imp-def,dead>, %SP<imp-use>
# End machine code for function f2.
*** Bad machine code: Reading virtual register without a def ***
- function: f2
- basic block: BB#1 lor.rhs.i (0x4dad4b8)
- instruction: %vreg10<def,tied1> = SEXT_rus
- operand 1: %vreg9<tied0>
LLVM ERROR: Found 1 machine code errors.
As a side note, while debugging I also found it a bit disturbing that
isNot accepts xors with all ones in both operand 0 and operand 1:
bool BinaryOperator::isNot(const Value *V) {
if (const BinaryOperator *Bop = dyn_cast<BinaryOperator>(V))
return (Bop->getOpcode() == Instruction::Xor &&
(isConstantAllOnes(Bop->getOperand(1)) ||
isConstantAllOnes(Bop->getOperand(0))));
return false;
}
but the new code seems to assume the ones are always in operand 1:
> + if (BinaryOperator::isNot(Cond) && Cond->hasOneUse()) {
> + Cond = cast<Instruction>(Cond)->getOperand(0);
But maybe I'm missing something here that maeks this always safe?
Regards,
Mikael
On 01/24/2017 05:36 PM, Geoff Berry via llvm-commits wrote:
> Author: gberry
> Date: Tue Jan 24 10:36:07 2017
> New Revision: 292944
>
> URL: http://llvm.org/viewvc/llvm-project?rev=292944&view=rev
> Log:
> [SelectionDAG] Handle inverted conditions when splitting into multiple branches.
>
> Summary:
> When conditional branches with complex conditions are split into
> multiple branches in SelectionDAGBuilder::FindMergedConditions, also
> handle inverted conditions. These may sometimes appear without having
> been optimized by InstCombine when CodeGenPrepare decides to sink and
> duplicate cmp instructions, causing them to have only one use. This
> problem can be increased by e.g. GVNHoist hiding more cmps from
> InstCombine by combining equivalent cmps from different blocks.
>
> For example codegen X & !(Y | Z) as:
> jmp_if_X TmpBB
> jmp FBB
> TmpBB:
> jmp_if_notY Tmp2BB
> jmp FBB
> Tmp2BB:
> jmp_if_notZ TBB
> jmp FBB
>
> Reviewers: bogner, MatzeB, qcolombet
>
> Subscribers: llvm-commits, hiraditya, mcrosier, sebpop
>
> Differential Revision: https://reviews.llvm.org/D28380
>
> Added:
> llvm/trunk/test/CodeGen/AArch64/br-cond-not-merge.ll
> Modified:
> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> llvm/trunk/test/CodeGen/X86/cmov.ll
> llvm/trunk/test/CodeGen/X86/dagcombine-and-setcc.ll
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp?rev=292944&r1=292943&r2=292944&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Tue Jan 24 10:36:07 2017
> @@ -1584,7 +1584,8 @@ SelectionDAGBuilder::EmitBranchForMerged
> MachineBasicBlock *CurBB,
> MachineBasicBlock *SwitchBB,
> BranchProbability TProb,
> - BranchProbability FProb) {
> + BranchProbability FProb,
> + bool InvertCond) {
> const BasicBlock *BB = CurBB->getBasicBlock();
>
> // If the leaf of the tree is a comparison, merge the condition into
> @@ -1598,10 +1599,14 @@ SelectionDAGBuilder::EmitBranchForMerged
> isExportableFromCurrentBlock(BOp->getOperand(1), BB))) {
> ISD::CondCode Condition;
> if (const ICmpInst *IC = dyn_cast<ICmpInst>(Cond)) {
> - Condition = getICmpCondCode(IC->getPredicate());
> + ICmpInst::Predicate Pred =
> + InvertCond ? IC->getInversePredicate() : IC->getPredicate();
> + Condition = getICmpCondCode(Pred);
> } else {
> const FCmpInst *FC = cast<FCmpInst>(Cond);
> - Condition = getFCmpCondCode(FC->getPredicate());
> + FCmpInst::Predicate Pred =
> + InvertCond ? FC->getInversePredicate() : FC->getPredicate();
> + Condition = getFCmpCondCode(Pred);
> if (TM.Options.NoNaNsFPMath)
> Condition = getFCmpCodeWithoutNaN(Condition);
> }
> @@ -1614,7 +1619,8 @@ SelectionDAGBuilder::EmitBranchForMerged
> }
>
> // Create a CaseBlock record representing this branch.
> - CaseBlock CB(ISD::SETEQ, Cond, ConstantInt::getTrue(*DAG.getContext()),
> + ISD::CondCode Opc = InvertCond ? ISD::SETNE : ISD::SETEQ;
> + CaseBlock CB(Opc, Cond, ConstantInt::getTrue(*DAG.getContext()),
> nullptr, TBB, FBB, CurBB, TProb, FProb);
> SwitchCases.push_back(CB);
> }
> @@ -1627,16 +1633,42 @@ void SelectionDAGBuilder::FindMergedCond
> MachineBasicBlock *SwitchBB,
> Instruction::BinaryOps Opc,
> BranchProbability TProb,
> - BranchProbability FProb) {
> - // If this node is not part of the or/and tree, emit it as a branch.
> + BranchProbability FProb,
> + bool InvertCond) {
> + // Skip over not part of the tree and remember to invert op and operands at
> + // next level.
> + if (BinaryOperator::isNot(Cond) && Cond->hasOneUse()) {
> + Cond = cast<Instruction>(Cond)->getOperand(0);
> + FindMergedConditions(Cond, TBB, FBB, CurBB, SwitchBB, Opc, TProb, FProb,
> + !InvertCond);
> + return;
> + }
> +
> const Instruction *BOp = dyn_cast<Instruction>(Cond);
> + // Compute the effective opcode for Cond, taking into account whether it needs
> + // to be inverted, e.g.
> + // and (not (or A, B)), C
> + // gets lowered as
> + // and (and (not A, not B), C)
> + unsigned BOpc = 0;
> + if (BOp) {
> + BOpc = BOp->getOpcode();
> + if (InvertCond) {
> + if (BOpc == Instruction::And)
> + BOpc = Instruction::Or;
> + else if (BOpc == Instruction::Or)
> + BOpc = Instruction::And;
> + }
> + }
> +
> + // If this node is not part of the or/and tree, emit it as a branch.
> if (!BOp || !(isa<BinaryOperator>(BOp) || isa<CmpInst>(BOp)) ||
> - (unsigned)BOp->getOpcode() != Opc || !BOp->hasOneUse() ||
> + BOpc != Opc || !BOp->hasOneUse() ||
> BOp->getParent() != CurBB->getBasicBlock() ||
> !InBlock(BOp->getOperand(0), CurBB->getBasicBlock()) ||
> !InBlock(BOp->getOperand(1), CurBB->getBasicBlock())) {
> EmitBranchForMergedCondition(Cond, TBB, FBB, CurBB, SwitchBB,
> - TProb, FProb);
> + TProb, FProb, InvertCond);
> return;
> }
>
> @@ -1671,14 +1703,14 @@ void SelectionDAGBuilder::FindMergedCond
> auto NewFalseProb = TProb / 2 + FProb;
> // Emit the LHS condition.
> FindMergedConditions(BOp->getOperand(0), TBB, TmpBB, CurBB, SwitchBB, Opc,
> - NewTrueProb, NewFalseProb);
> + NewTrueProb, NewFalseProb, InvertCond);
>
> // Normalize A/2 and B to get A/(1+B) and 2B/(1+B).
> SmallVector<BranchProbability, 2> Probs{TProb / 2, FProb};
> BranchProbability::normalizeProbabilities(Probs.begin(), Probs.end());
> // Emit the RHS condition into TmpBB.
> FindMergedConditions(BOp->getOperand(1), TBB, FBB, TmpBB, SwitchBB, Opc,
> - Probs[0], Probs[1]);
> + Probs[0], Probs[1], InvertCond);
> } else {
> assert(Opc == Instruction::And && "Unknown merge op!");
> // Codegen X & Y as:
> @@ -1704,14 +1736,14 @@ void SelectionDAGBuilder::FindMergedCond
> auto NewFalseProb = FProb / 2;
> // Emit the LHS condition.
> FindMergedConditions(BOp->getOperand(0), TmpBB, FBB, CurBB, SwitchBB, Opc,
> - NewTrueProb, NewFalseProb);
> + NewTrueProb, NewFalseProb, InvertCond);
>
> // Normalize A and B/2 to get 2A/(1+A) and B/(1+A).
> SmallVector<BranchProbability, 2> Probs{TProb, FProb / 2};
> BranchProbability::normalizeProbabilities(Probs.begin(), Probs.end());
> // Emit the RHS condition into TmpBB.
> FindMergedConditions(BOp->getOperand(1), TBB, FBB, TmpBB, SwitchBB, Opc,
> - Probs[0], Probs[1]);
> + Probs[0], Probs[1], InvertCond);
> }
> }
>
> @@ -1795,7 +1827,8 @@ void SelectionDAGBuilder::visitBr(const
> FindMergedConditions(BOp, Succ0MBB, Succ1MBB, BrMBB, BrMBB,
> Opcode,
> getEdgeProbability(BrMBB, Succ0MBB),
> - getEdgeProbability(BrMBB, Succ1MBB));
> + getEdgeProbability(BrMBB, Succ1MBB),
> + /*InvertCond=*/false);
> // If the compares in later blocks need to use values not currently
> // exported from this block, export them now. This block should always
> // be the first entry.
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h?rev=292944&r1=292943&r2=292944&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h Tue Jan 24 10:36:07 2017
> @@ -688,12 +688,13 @@ public:
> MachineBasicBlock *FBB, MachineBasicBlock *CurBB,
> MachineBasicBlock *SwitchBB,
> Instruction::BinaryOps Opc, BranchProbability TW,
> - BranchProbability FW);
> + BranchProbability FW, bool InvertCond);
> void EmitBranchForMergedCondition(const Value *Cond, MachineBasicBlock *TBB,
> MachineBasicBlock *FBB,
> MachineBasicBlock *CurBB,
> MachineBasicBlock *SwitchBB,
> - BranchProbability TW, BranchProbability FW);
> + BranchProbability TW, BranchProbability FW,
> + bool InvertCond);
> bool ShouldEmitAsBranches(const std::vector<CaseBlock> &Cases);
> bool isExportableFromCurrentBlock(const Value *V, const BasicBlock *FromBB);
> void CopyToExportRegsIfNeeded(const Value *V);
>
> Added: llvm/trunk/test/CodeGen/AArch64/br-cond-not-merge.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/br-cond-not-merge.ll?rev=292944&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/AArch64/br-cond-not-merge.ll (added)
> +++ llvm/trunk/test/CodeGen/AArch64/br-cond-not-merge.ll Tue Jan 24 10:36:07 2017
> @@ -0,0 +1,32 @@
> +; RUN: llc -mtriple=aarch64 -verify-machineinstrs < %s | FileCheck %s
> +
> +declare void @foo()
> +
> +; Check that the inverted or doesn't inhibit the splitting of the
> +; complex conditional into three branch instructions.
> +; CHECK-LABEL: test_and_not
> +; CHECK: cbz w0, [[L:\.LBB[0-9_]+]]
> +; CHECK: cmp w1, #2
> +; CHECK: b.lo [[L]]
> +; CHECK: cmp w2, #2
> +; CHECK: b.hi [[L]]
> +define void @test_and_not(i32 %a, i32 %b, i32 %c) {
> +bb1:
> + %cmp1 = icmp ult i32 %a, 1
> + %cmp2 = icmp ult i32 %b, 2
> + %cmp3 = icmp ult i32 %c, 3
> + %or = or i1 %cmp1, %cmp2
> + %not.or = xor i1 %or, -1
> + %and = and i1 %not.or, %cmp3
> + br i1 %and, label %bb2, label %bb3
> +
> +bb2:
> + ret void
> +
> +bb3:
> + call void @foo()
> + ret void
> +}
> +
> +
> +
>
> Modified: llvm/trunk/test/CodeGen/X86/cmov.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/cmov.ll?rev=292944&r1=292943&r2=292944&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/cmov.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/cmov.ll Tue Jan 24 10:36:07 2017
> @@ -70,7 +70,7 @@ define void @test3(i64 %a, i64 %b, i1 %p
> @g_100 = external global i8 ; <i8*> [#uses=2]
> @_2E_str = external constant [15 x i8], align 1 ; <[15 x i8]*> [#uses=1]
>
> -define i32 @test4() nounwind {
> +define i1 @test4() nounwind {
> entry:
> %0 = load i8, i8* @g_3, align 1 ; <i8> [#uses=2]
> %1 = sext i8 %0 to i32 ; <i32> [#uses=1]
> @@ -107,10 +107,11 @@ bb.i.i:
>
> func_1.exit: ; preds = %bb.i.i, %func_4.exit.i
> %g_96.tmp.0.i = phi i8 [ %g_96.promoted.i, %bb.i.i ], [ %.mux.i, %func_4.exit.i ] ; <i8> [#uses=2]
> + %ret = phi i1 [ 0, %bb.i.i ], [ %.not.i, %func_4.exit.i ]
> store i8 %g_96.tmp.0.i, i8* @g_96
> %6 = zext i8 %g_96.tmp.0.i to i32 ; <i32> [#uses=1]
> %7 = tail call i32 (i8*, ...) @printf(i8* noalias getelementptr ([15 x i8], [15 x i8]* @_2E_str, i64 0, i64 0), i32 %6) nounwind ; <i32> [#uses=0]
> - ret i32 0
> + ret i1 %ret
> }
>
> declare i32 @printf(i8* nocapture, ...) nounwind
>
> Modified: llvm/trunk/test/CodeGen/X86/dagcombine-and-setcc.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/dagcombine-and-setcc.ll?rev=292944&r1=292943&r2=292944&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/dagcombine-and-setcc.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/dagcombine-and-setcc.ll Tue Jan 24 10:36:07 2017
> @@ -12,10 +12,11 @@ declare i32 @printf(i8* nocapture readon
>
>
> ;CHECK: cmpl
> -;CHECK: setg
> +;CHECK: setl
> ;CHECK: cmpl
> -;CHECK: setg
> -;CHECK: andb
> +;CHECK: setl
> +;CHECK: orb
> +;CHECK: je
>
> @.str = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
> ; Function Attrs: optsize ssp uwtable
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
@a = external local_unnamed_addr global i16, align 1
; Function Attrs: norecurse nounwind
define void @f2() {
entry:
%0 = load i16, i16* @a, align 1
%cmp = icmp sgt i16 %0, 0
br label %lor.rhs.i
lor.rhs.i: ; preds = %entry
%cmp.i = xor i1 %cmp, true
%or.cond1.i = and i1 %cmp.i, icmp ne (i16 ptrtoint (void () * @f2 to i16), i16 0)
br i1 %or.cond1.i, label %f1.exit, label %f1.exit
f1.exit: ; preds = %lor.rhs.i
ret void
}
More information about the llvm-commits
mailing list