From 7e18cd887cd402e3d5465c57c218079e4df65231 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Sat, 20 Mar 2021 21:01:49 +0100 Subject: [PATCH] [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 --- llvm/include/llvm/Analysis/InstructionSimplify.h | 4 +- llvm/lib/Analysis/InstructionSimplify.cpp | 49 ++++++++++++++++++------ llvm/test/Transforms/InstCombine/minmax-fold.ll | 2 +- llvm/test/Transforms/InstCombine/select.ll | 4 +- llvm/test/Transforms/InstSimplify/pr49495.ll | 16 +++++--- 5 files changed, 54 insertions(+), 21 deletions(-) diff --git a/llvm/include/llvm/Analysis/InstructionSimplify.h b/llvm/include/llvm/Analysis/InstructionSimplify.h index 17d6f30..dda90e8 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 4d7e281..1dc7499 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(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(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(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(I))) + return nullptr; + if (CmpInst *C = dyn_cast(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 7812630..7aa3a39 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 ad1d329..f98a369 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 f085de3..b3eca96 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 -- 2.7.4