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