[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 22:52:04 PST 2017
Hi,
On 02/07/2017 10:20 PM, Geoff Berry wrote:
> 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?
Yes now it compiles succesfully. Great!
Thanks,
Mikael
>
> 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
>>>
>
More information about the llvm-commits
mailing list