[llvm] d7267ee - [ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into branch conditions of dominating blocks' terminators
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 5 08:59:00 PST 2020
On Thu, Mar 5, 2020 at 7:56 PM Philip Reames <listmail at philipreames.com> wrote:
>
>
> On 3/5/20 8:13 AM, Roman Lebedev via llvm-commits wrote:
> > On Thu, Mar 5, 2020 at 7:09 PM Juneyoung Lee via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: Juneyoung Lee
> >> Date: 2020-03-06T01:08:35+09:00
> >> New Revision: d7267ee1941668d7e985681e29d10983799811a0
> >>
> >> URL: https://github.com/llvm/llvm-project/commit/d7267ee1941668d7e985681e29d10983799811a0
> >> DIFF: https://github.com/llvm/llvm-project/commit/d7267ee1941668d7e985681e29d10983799811a0.diff
> >>
> >> LOG: [ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into branch conditions of dominating blocks' terminators
> >>
> >> Summary:
> >> ```
> >> br i1 c, BB1, BB2:
> >> BB1:
> >> use1(c)
> >> BB2:
> >> use2(c)
> >> ```
> >>
> >> In BB1 and BB2, c is never undef or poison because otherwise the branch would have triggered UB.
> >>
> >> This is a resubmission of 952ad47 with crash fix of llvm/test/Transforms/LoopRotate/freeze-crash.ll.
> >>
> >> Checked with Alive2
> >>
> >> Reviewers: xbolva00, spatel, lebedev.ri, reames, jdoerfert, nlopes, sanjoy
> >>
> >> Reviewed By: reames
> >>
> >> Subscribers: jdoerfert, hiraditya, llvm-commits
> >>
> >> Tags: #llvm
> >>
> >> Differential Revision: https://reviews.llvm.org/D75401
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >> llvm/include/llvm/Analysis/ValueTracking.h
> >> llvm/lib/Analysis/InstructionSimplify.cpp
> >> llvm/lib/Analysis/ValueTracking.cpp
> >> llvm/test/Transforms/InstSimplify/freeze.ll
> >>
> >> Removed:
> >>
> >>
> >>
> >> ################################################################################
> >> diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
> >> index 3a4fe8bdc199..dc28d2375a78 100644
> >> --- a/llvm/include/llvm/Analysis/ValueTracking.h
> >> +++ b/llvm/include/llvm/Analysis/ValueTracking.h
> >> @@ -569,7 +569,13 @@ class Value;
> >>
> >> /// Return true if this function can prove that V is never undef value
> >> /// or poison value.
> >> - bool isGuaranteedNotToBeUndefOrPoison(const Value *V);
> >> + //
> >> + /// If CtxI and DT are specified this method performs flow-sensitive analysis
> >> + /// and returns true if it is guaranteed to be never undef or poison
> >> + /// immediately before the CtxI.
> >> + bool isGuaranteedNotToBeUndefOrPoison(const Value *V,
> >> + const Instruction *CtxI = nullptr,
> >> + const DominatorTree *DT = nullptr);
> >>
> >> /// Specific patterns of select instructions we can match.
> >> enum SelectPatternFlavor {
> >>
> >> diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
> >> index 9fa78f369d0e..137f7455b65a 100644
> >> --- a/llvm/lib/Analysis/InstructionSimplify.cpp
> >> +++ b/llvm/lib/Analysis/InstructionSimplify.cpp
> >> @@ -5360,16 +5360,16 @@ Value *llvm::SimplifyCall(CallBase *Call, const SimplifyQuery &Q) {
> >> }
> >>
> >> /// Given operands for a Freeze, see if we can fold the result.
> >> -static Value *SimplifyFreezeInst(Value *Op0) {
> >> +static Value *SimplifyFreezeInst(Value *Op0, const SimplifyQuery &Q) {
> >> // Use a utility function defined in ValueTracking.
> >> - if (llvm::isGuaranteedNotToBeUndefOrPoison(Op0))
> >> + if (llvm::isGuaranteedNotToBeUndefOrPoison(Op0, Q.CxtI, Q.DT))
> >> return Op0;
> >> // We have room for improvement.
> >> return nullptr;
> >> }
> >>
> >> Value *llvm::SimplifyFreezeInst(Value *Op0, const SimplifyQuery &Q) {
> >> - return ::SimplifyFreezeInst(Op0);
> >> + return ::SimplifyFreezeInst(Op0, Q);
> >> }
> >>
> >> /// See if we can compute a simplified version of this instruction.
> >>
> >> diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
> >> index e11f5e2a8397..d46a5d998f5b 100644
> >> --- a/llvm/lib/Analysis/ValueTracking.cpp
> >> +++ b/llvm/lib/Analysis/ValueTracking.cpp
> >> @@ -4525,7 +4525,9 @@ bool llvm::isOverflowIntrinsicNoWrap(const WithOverflowInst *WO,
> >> return llvm::any_of(GuardingBranches, AllUsesGuardedByBranch);
> >> }
> >>
> >> -bool llvm::isGuaranteedNotToBeUndefOrPoison(const Value *V) {
> >> +bool llvm::isGuaranteedNotToBeUndefOrPoison(const Value *V,
> >> + const Instruction *CtxI,
> >> + const DominatorTree *DT) {
> >> // If the value is a freeze instruction, then it can never
> >> // be undef or poison.
> >> if (isa<FreezeInst>(V))
> >> @@ -4558,6 +4560,30 @@ bool llvm::isGuaranteedNotToBeUndefOrPoison(const Value *V) {
> >> return true;
> >> }
> >>
> >> + // CxtI may be null or a cloned instruction.
> >> + if (!CtxI || !CtxI->getParent() || !DT)
> >> + return false;
> >> +
> >> + // If V is used as a branch condition before reaching CtxI, V cannot be
> >> + // undef or poison.
> >> + // br V, BB1, BB2
> >> + // BB1:
> >> + // CtxI ; V cannot be undef or poison here
> >> + auto Dominator = DT->getNode(CtxI->getParent())->getIDom();
> >> + while (Dominator) {
> >> + auto *TI = Dominator->getBlock()->getTerminator();
> >> +
> >> + if (auto BI = dyn_cast<BranchInst>(TI)) {
> >> + if (BI->isConditional() && BI->getCondition() == V)
> >> + return true;
> >> + } else if (auto SI = dyn_cast<SwitchInst>(TI)) {
> >> + if (SI->getCondition() == V)
> >> + return true;
> >> + }
> >> +
> >> + Dominator = Dominator->getIDom();
> >> + }
> >> +
> >> return false;
> >> }
> >>
> >>
> >> diff --git a/llvm/test/Transforms/InstSimplify/freeze.ll b/llvm/test/Transforms/InstSimplify/freeze.ll
> >> index ac287b949f07..e6085bf392bf 100644
> >> --- a/llvm/test/Transforms/InstSimplify/freeze.ll
> >> +++ b/llvm/test/Transforms/InstSimplify/freeze.ll
> >> @@ -18,3 +18,66 @@ define i32 @make_const() {
> >> %x = freeze i32 10
> >> ret i32 %x
> >> }
> >> +
> >> +define i1 @brcond(i1 %c, i1 %c2) {
> >> +; CHECK-LABEL: @brcond(
> >> +; CHECK-NEXT: br i1 [[C:%.*]], label [[A:%.*]], label [[B:%.*]]
> >> +; CHECK: A:
> >> +; CHECK-NEXT: br i1 [[C2:%.*]], label [[A2:%.*]], label [[B]]
> >> +; CHECK: A2:
> >> +; CHECK-NEXT: ret i1 [[C]]
> >> +; CHECK: B:
> >> +; CHECK-NEXT: ret i1 [[C]]
> >> +;
> >> + br i1 %c, label %A, label %B
> >> +A:
> >> + br i1 %c2, label %A2, label %B
> >> +A2:
> >> + %f1 = freeze i1 %c
> >> + ret i1 %f1
> >> +B:
> >> + %f2 = freeze i1 %c
> >> + ret i1 %f2
> >> +}
> >> +
> >> +define i32 @brcond_switch(i32 %x) {
> >> +; CHECK-LABEL: @brcond_switch(
> >> +; CHECK-NEXT: switch i32 [[X:%.*]], label [[EXIT:%.*]] [
> >> +; CHECK-NEXT: i32 0, label [[A:%.*]]
> >> +; CHECK-NEXT: ]
> >> +; CHECK: A:
> >> +; CHECK-NEXT: ret i32 [[X]]
> >> +; CHECK: EXIT:
> >> +; CHECK-NEXT: ret i32 [[X]]
> >> +;
> >> + switch i32 %x, label %EXIT [ i32 0, label %A ]
> >> +A:
> >> + %fr1 = freeze i32 %x
> >> + ret i32 %fr1
> >> +EXIT:
> >> + %fr2 = freeze i32 %x
> >> + ret i32 %fr2
> >> +}
> >> +
> >> +define i1 @brcond_noopt(i1 %c, i1 %c2) {
> >> +; CHECK-LABEL: @brcond_noopt(
> >> +; CHECK-NEXT: [[F:%.*]] = freeze i1 [[C:%.*]]
> >> +; CHECK-NEXT: call void @f1(i1 [[F]])
> >> +; CHECK-NEXT: call void @f2()
> >> +; CHECK-NEXT: br i1 [[C]], label [[A:%.*]], label [[B:%.*]]
> >> +; CHECK: A:
> >> +; CHECK-NEXT: ret i1 false
> >> +; CHECK: B:
> >> +; CHECK-NEXT: ret i1 true
> >> +;
> >> + %f = freeze i1 %c
> >> + call void @f1(i1 %f) ; cannot optimize i1 %f to %c
> >> + call void @f2() ; .. because if f2() exits, `br %c` cannot be reached
> >> + br i1 %c, label %A, label %B
> >> +A:
> >> + ret i1 0
> >> +B:
> >> + ret i1 1
> >> +}
> >> +declare void @f1(i1)
> >> +declare void @f2()
> > In all future patches, please, unless the testcase is crashing without
> > the patch, please do commit the testcase first, so that the patch
> > only shows the changed check-lines in the tests, not just introduces
> > whole new tests.
> > This way the tests won't get lost when the patch is reverted, too.
>
> I don't think this is required. It's definitely good practice, but not
> required.
Yes
> I make the point only because doing this for auto-generated tests is
> easy, but for tests which can't be auto-gened it's tedious and error prone.
I was indeed only talking about auto-generated tests.
> >
> >
> > Roman
> >
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list