[llvm] 7e18cd8 - [InstCombine] Whitelist non-refining folds in SimplifyWithOpReplaced
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 23 14:50:51 PDT 2021
On Tue, Mar 23, 2021 at 5:55 PM Philip Reames <listmail at philipreames.com>
wrote:
> Is there a high level description of the problem AllowRefinement is
> trying to solve somewhere? I don't follow the intent of this, and would
> like to read up on the background.
>
> Philip
>
The transform in question does basically this:
x == -1 ? 0 : x + 1
same as
x == -1 ? x + 1 : x + 1
same as
x + 1
Note that it does something very unusual: Rather than replacing x + 1 (for
x=-1) with its simplification result 0, it instead replaces the
simplification result 0 with x + 1 (for x=-1). This is a problem, because
simplifications are generally only one way, due to refinement. It's okay to
"refine" poison to 0, but not 0 to poison.
So if we look at the case "x == -1 ? 0 : x +<nuw> 1", we run into a
problem: InstSimplify tells us that x +<nuw> 1 (for x=-1) simplifies to 0
-- but that doesn't mean we can replace 0 with x +<nuw> 1. The actual value
of that expression is poison in this case, so we'd end up replacing 0 with
poison.
This is what the AllowRefinement flag here controls. Another view on this
would be that !AllowRefinement == MustBeInvertible.
Regards,
Nikita
> On 3/22/21 2:13 PM, Nikita Popov via llvm-commits wrote:
> > Author: Nikita Popov
> > Date: 2021-03-22T22:12:56+01:00
> > New Revision: 7e18cd887cd402e3d5465c57c218079e4df65231
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/7e18cd887cd402e3d5465c57c218079e4df65231
> > DIFF:
> https://github.com/llvm/llvm-project/commit/7e18cd887cd402e3d5465c57c218079e4df65231.diff
> >
> > LOG: [InstCombine] Whitelist non-refining folds in SimplifyWithOpReplaced
> >
> > This is an alternative to D98391/D98585, playing things more
> > conservatively. If AllowRefinement == false, then we don't use
> > InstSimplify methods at all, and instead explicitly implement a
> > small number of non-refining folds. Most cases are handled by
> > constant folding, and I only had to add three folds to cover
> > our unit tests / test-suite. While this may lose some optimization
> > power, I think it is safer to approach from this direction, given
> > how many issues this code has already caused.
> >
> > Differential Revision: https://reviews.llvm.org/D99027
> >
> > Added:
> >
> >
> > Modified:
> > llvm/include/llvm/Analysis/InstructionSimplify.h
> > llvm/lib/Analysis/InstructionSimplify.cpp
> > llvm/test/Transforms/InstCombine/minmax-fold.ll
> > llvm/test/Transforms/InstCombine/select.ll
> > llvm/test/Transforms/InstSimplify/pr49495.ll
> >
> > Removed:
> >
> >
> >
> >
> ################################################################################
> > diff --git a/llvm/include/llvm/Analysis/InstructionSimplify.h
> b/llvm/include/llvm/Analysis/InstructionSimplify.h
> > index 17d6f30a35cb..dda90e826bba 100644
> > --- a/llvm/include/llvm/Analysis/InstructionSimplify.h
> > +++ b/llvm/include/llvm/Analysis/InstructionSimplify.h
> > @@ -294,8 +294,8 @@ Value *SimplifyInstruction(Instruction *I, const
> SimplifyQuery &Q,
> >
> > /// See if V simplifies when its operand Op is replaced with RepOp. If
> not,
> > /// return null.
> > -/// AllowRefinement specifies whether the simplification can be a
> refinement,
> > -/// or whether it needs to be strictly identical.
> > +/// AllowRefinement specifies whether the simplification can be a
> refinement
> > +/// (e.g. 0 instead of poison), or whether it needs to be strictly
> identical.
> > Value *SimplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
> > const SimplifyQuery &Q, bool
> AllowRefinement);
> >
> >
> > diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp
> b/llvm/lib/Analysis/InstructionSimplify.cpp
> > index 4d7e281312ba..1dc7499c85c8 100644
> > --- a/llvm/lib/Analysis/InstructionSimplify.cpp
> > +++ b/llvm/lib/Analysis/InstructionSimplify.cpp
> > @@ -3936,18 +3936,33 @@ static Value *SimplifyWithOpReplaced(Value *V,
> Value *Op, Value *RepOp,
> > transform(I->operands(), NewOps.begin(),
> > [&](Value *V) { return V == Op ? RepOp : V; });
> >
> > - // Consider:
> > - // %cmp = icmp eq i32 %x, 2147483647
> > - // %add = add nsw i32 %x, 1
> > - // %sel = select i1 %cmp, i32 -2147483648, i32 %add
> > - //
> > - // We can't replace %sel with %add unless we strip away the flags
> (which will
> > - // be done in InstCombine).
> > - // TODO: This is unsound, because it only catches some forms of
> refinement.
> > - if (!AllowRefinement && canCreatePoison(cast<Operator>(I)))
> > - return nullptr;
> > + if (!AllowRefinement) {
> > + // General InstSimplify functions may refine the result, e.g. by
> returning
> > + // a constant for a potentially poison value. To avoid this,
> implement only
> > + // a few non-refining but profitable transforms here.
> > +
> > + if (auto *BO = dyn_cast<BinaryOperator>(I)) {
> > + unsigned Opcode = BO->getOpcode();
> > + // id op x -> x, x op id -> x
> > + if (NewOps[0] == ConstantExpr::getBinOpIdentity(Opcode,
> I->getType()))
> > + return NewOps[1];
> > + if (NewOps[1] == ConstantExpr::getBinOpIdentity(Opcode,
> I->getType(),
> > + /* RHS */ true))
> > + return NewOps[0];
> > +
> > + // x & x -> x, x | x -> x
> > + if ((Opcode == Instruction::And || Opcode == Instruction::Or) &&
> > + NewOps[0] == NewOps[1])
> > + return NewOps[0];
> > + }
> >
> > - if (MaxRecurse) {
> > + if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) {
> > + // getelementptr x, 0 -> x
> > + if (NewOps.size() == 2 && match(NewOps[1], m_Zero()) &&
> > + !GEP->isInBounds())
> > + return NewOps[0];
> > + }
> > + } else if (MaxRecurse) {
> > // The simplification queries below may return the original value.
> Consider:
> > // %div = udiv i32 %arg, %arg2
> > // %mul = mul nsw i32 %div, %arg2
> > @@ -3986,6 +4001,18 @@ static Value *SimplifyWithOpReplaced(Value *V,
> Value *Op, Value *RepOp,
> > return nullptr;
> > }
> >
> > + // Consider:
> > + // %cmp = icmp eq i32 %x, 2147483647
> > + // %add = add nsw i32 %x, 1
> > + // %sel = select i1 %cmp, i32 -2147483648, i32 %add
> > + //
> > + // We can't replace %sel with %add unless we strip away the flags
> (which
> > + // will be done in InstCombine).
> > + // TODO: This may be unsound, because it only catches some forms of
> > + // refinement.
> > + if (!AllowRefinement && canCreatePoison(cast<Operator>(I)))
> > + return nullptr;
> > +
> > if (CmpInst *C = dyn_cast<CmpInst>(I))
> > return ConstantFoldCompareInstOperands(C->getPredicate(),
> ConstOps[0],
> > ConstOps[1], Q.DL, Q.TLI);
> >
> > diff --git a/llvm/test/Transforms/InstCombine/minmax-fold.ll
> b/llvm/test/Transforms/InstCombine/minmax-fold.ll
> > index 78126302f9b9..7aa3a3992449 100644
> > --- a/llvm/test/Transforms/InstCombine/minmax-fold.ll
> > +++ b/llvm/test/Transforms/InstCombine/minmax-fold.ll
> > @@ -1080,7 +1080,7 @@ define i37 @add_umax_constant_limit(i37 %x) {
> >
> > define i37 @add_umax_simplify(i37 %x) {
> > ; CHECK-LABEL: @add_umax_simplify(
> > -; CHECK-NEXT: [[A:%.*]] = add i37 [[X:%.*]], 42
> > +; CHECK-NEXT: [[A:%.*]] = add nuw i37 [[X:%.*]], 42
> > ; CHECK-NEXT: ret i37 [[A]]
> > ;
> > %a = add nuw i37 %x, 42
> >
> > diff --git a/llvm/test/Transforms/InstCombine/select.ll
> b/llvm/test/Transforms/InstCombine/select.ll
> > index ad1d32999556..f98a369c144b 100644
> > --- a/llvm/test/Transforms/InstCombine/select.ll
> > +++ b/llvm/test/Transforms/InstCombine/select.ll
> > @@ -902,7 +902,9 @@ define i32 @test56(i16 %x) {
> > define i32 @test57(i32 %x, i32 %y) {
> > ; CHECK-LABEL: @test57(
> > ; CHECK-NEXT: [[AND:%.*]] = and i32 [[X:%.*]], [[Y:%.*]]
> > -; CHECK-NEXT: ret i32 [[AND]]
> > +; CHECK-NEXT: [[TOBOOL:%.*]] = icmp eq i32 [[X]], 0
> > +; CHECK-NEXT: [[DOTAND:%.*]] = select i1 [[TOBOOL]], i32 0, i32
> [[AND]]
> > +; CHECK-NEXT: ret i32 [[DOTAND]]
> > ;
> > %and = and i32 %x, %y
> > %tobool = icmp eq i32 %x, 0
> >
> > diff --git a/llvm/test/Transforms/InstSimplify/pr49495.ll
> b/llvm/test/Transforms/InstSimplify/pr49495.ll
> > index f085de3b4a22..b3eca968c060 100644
> > --- a/llvm/test/Transforms/InstSimplify/pr49495.ll
> > +++ b/llvm/test/Transforms/InstSimplify/pr49495.ll
> > @@ -4,9 +4,11 @@
> > ; The first comparison (a != b) should not be dropped
> > define i1 @test1(i8* %a, i8* %b) {
> > ; CHECK-LABEL: @test1(
> > -; CHECK-NEXT: [[A2:%.*]] = getelementptr inbounds i8, i8* [[A:%.*]],
> i64 -1
> > -; CHECK-NEXT: [[COND2:%.*]] = icmp ugt i8* [[A2]], [[B:%.*]]
> > -; CHECK-NEXT: ret i1 [[COND2]]
> > +; CHECK-NEXT: [[COND1:%.*]] = icmp ne i8* [[A:%.*]], [[B:%.*]]
> > +; CHECK-NEXT: [[A2:%.*]] = getelementptr inbounds i8, i8* [[A]], i64
> -1
> > +; CHECK-NEXT: [[COND2:%.*]] = icmp ugt i8* [[A2]], [[B]]
> > +; CHECK-NEXT: [[RES:%.*]] = select i1 [[COND1]], i1 [[COND2]], i1
> false
> > +; CHECK-NEXT: ret i1 [[RES]]
> > ;
> > %cond1 = icmp ne i8* %a, %b
> > %a2 = getelementptr inbounds i8, i8* %a, i64 -1
> > @@ -18,9 +20,11 @@ define i1 @test1(i8* %a, i8* %b) {
> > ; The first comparison (a != b) should not be dropped
> > define i1 @test2(i32 %a, i32 %b) {
> > ; CHECK-LABEL: @test2(
> > -; CHECK-NEXT: [[A2:%.*]] = add nuw i32 [[A:%.*]], 1
> > -; CHECK-NEXT: [[COND2:%.*]] = icmp ult i32 [[A2]], [[B:%.*]]
> > -; CHECK-NEXT: ret i1 [[COND2]]
> > +; CHECK-NEXT: [[COND1:%.*]] = icmp ne i32 [[A:%.*]], [[B:%.*]]
> > +; CHECK-NEXT: [[A2:%.*]] = add nuw i32 [[A]], 1
> > +; CHECK-NEXT: [[COND2:%.*]] = icmp ult i32 [[A2]], [[B]]
> > +; CHECK-NEXT: [[RES:%.*]] = select i1 [[COND1]], i1 [[COND2]], i1
> false
> > +; CHECK-NEXT: ret i1 [[RES]]
> > ;
> > %cond1 = icmp ne i32 %a, %b
> > %a2 = add nuw i32 %a, 1
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210323/50a12481/attachment-0001.html>
More information about the llvm-commits
mailing list