[llvm] r292944 - [SelectionDAG] Handle inverted conditions when splitting into multiple branches.
Geoff Berry via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 13:20:48 PST 2017
Mikael,
Thanks for looking into this. I put a patch that should fix both issues
up for review: https://reviews.llvm.org/D29680
Can you verify that this fixes your original issue for your target?
Thanks,
-Geoff
On 2/7/2017 5:41 AM, Mikael Holmén wrote:
> 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
>>
--
Geoff Berry
Employee of Qualcomm Datacenter Technologies, Inc.
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
More information about the llvm-commits
mailing list