Skip to content

Commit fd6bd7a

Browse files
nikicmemfrob
authored andcommitted
[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
1 parent a129bfa commit fd6bd7a

File tree

5 files changed

+54
-21
lines changed

5 files changed

+54
-21
lines changed

llvm/include/llvm/Analysis/InstructionSimplify.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,8 +294,8 @@ Value *SimplifyInstruction(Instruction *I, const SimplifyQuery &Q,
294294

295295
/// See if V simplifies when its operand Op is replaced with RepOp. If not,
296296
/// return null.
297-
/// AllowRefinement specifies whether the simplification can be a refinement,
298-
/// or whether it needs to be strictly identical.
297+
/// AllowRefinement specifies whether the simplification can be a refinement
298+
/// (e.g. 0 instead of poison), or whether it needs to be strictly identical.
299299
Value *SimplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
300300
const SimplifyQuery &Q, bool AllowRefinement);
301301

llvm/lib/Analysis/InstructionSimplify.cpp

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3936,18 +3936,33 @@ static Value *SimplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
39363936
transform(I->operands(), NewOps.begin(),
39373937
[&](Value *V) { return V == Op ? RepOp : V; });
39383938

3939-
// Consider:
3940-
// %cmp = icmp eq i32 %x, 2147483647
3941-
// %add = add nsw i32 %x, 1
3942-
// %sel = select i1 %cmp, i32 -2147483648, i32 %add
3943-
//
3944-
// We can't replace %sel with %add unless we strip away the flags (which will
3945-
// be done in InstCombine).
3946-
// TODO: This is unsound, because it only catches some forms of refinement.
3947-
if (!AllowRefinement && canCreatePoison(cast<Operator>(I)))
3948-
return nullptr;
3939+
if (!AllowRefinement) {
3940+
// General InstSimplify functions may refine the result, e.g. by returning
3941+
// a constant for a potentially poison value. To avoid this, implement only
3942+
// a few non-refining but profitable transforms here.
3943+
3944+
if (auto *BO = dyn_cast<BinaryOperator>(I)) {
3945+
unsigned Opcode = BO->getOpcode();
3946+
// id op x -> x, x op id -> x
3947+
if (NewOps[0] == ConstantExpr::getBinOpIdentity(Opcode, I->getType()))
3948+
return NewOps[1];
3949+
if (NewOps[1] == ConstantExpr::getBinOpIdentity(Opcode, I->getType(),
3950+
/* RHS */ true))
3951+
return NewOps[0];
3952+
3953+
// x & x -> x, x | x -> x
3954+
if ((Opcode == Instruction::And || Opcode == Instruction::Or) &&
3955+
NewOps[0] == NewOps[1])
3956+
return NewOps[0];
3957+
}
39493958

3950-
if (MaxRecurse) {
3959+
if (auto *GEP = dyn_cast<GetElementPtrInst>(I)) {
3960+
// getelementptr x, 0 -> x
3961+
if (NewOps.size() == 2 && match(NewOps[1], m_Zero()) &&
3962+
!GEP->isInBounds())
3963+
return NewOps[0];
3964+
}
3965+
} else if (MaxRecurse) {
39513966
// The simplification queries below may return the original value. Consider:
39523967
// %div = udiv i32 %arg, %arg2
39533968
// %mul = mul nsw i32 %div, %arg2
@@ -3986,6 +4001,18 @@ static Value *SimplifyWithOpReplaced(Value *V, Value *Op, Value *RepOp,
39864001
return nullptr;
39874002
}
39884003

4004+
// Consider:
4005+
// %cmp = icmp eq i32 %x, 2147483647
4006+
// %add = add nsw i32 %x, 1
4007+
// %sel = select i1 %cmp, i32 -2147483648, i32 %add
4008+
//
4009+
// We can't replace %sel with %add unless we strip away the flags (which
4010+
// will be done in InstCombine).
4011+
// TODO: This may be unsound, because it only catches some forms of
4012+
// refinement.
4013+
if (!AllowRefinement && canCreatePoison(cast<Operator>(I)))
4014+
return nullptr;
4015+
39894016
if (CmpInst *C = dyn_cast<CmpInst>(I))
39904017
return ConstantFoldCompareInstOperands(C->getPredicate(), ConstOps[0],
39914018
ConstOps[1], Q.DL, Q.TLI);

llvm/test/Transforms/InstCombine/minmax-fold.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1080,7 +1080,7 @@ define i37 @add_umax_constant_limit(i37 %x) {
10801080

10811081
define i37 @add_umax_simplify(i37 %x) {
10821082
; CHECK-LABEL: @add_umax_simplify(
1083-
; CHECK-NEXT: [[A:%.*]] = add i37 [[X:%.*]], 42
1083+
; CHECK-NEXT: [[A:%.*]] = add nuw i37 [[X:%.*]], 42
10841084
; CHECK-NEXT: ret i37 [[A]]
10851085
;
10861086
%a = add nuw i37 %x, 42

llvm/test/Transforms/InstCombine/select.ll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -902,7 +902,9 @@ define i32 @test56(i16 %x) {
902902
define i32 @test57(i32 %x, i32 %y) {
903903
; CHECK-LABEL: @test57(
904904
; CHECK-NEXT: [[AND:%.*]] = and i32 [[X:%.*]], [[Y:%.*]]
905-
; CHECK-NEXT: ret i32 [[AND]]
905+
; CHECK-NEXT: [[TOBOOL:%.*]] = icmp eq i32 [[X]], 0
906+
; CHECK-NEXT: [[DOTAND:%.*]] = select i1 [[TOBOOL]], i32 0, i32 [[AND]]
907+
; CHECK-NEXT: ret i32 [[DOTAND]]
906908
;
907909
%and = and i32 %x, %y
908910
%tobool = icmp eq i32 %x, 0

llvm/test/Transforms/InstSimplify/pr49495.ll

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@
44
; The first comparison (a != b) should not be dropped
55
define i1 @test1(i8* %a, i8* %b) {
66
; CHECK-LABEL: @test1(
7-
; CHECK-NEXT: [[A2:%.*]] = getelementptr inbounds i8, i8* [[A:%.*]], i64 -1
8-
; CHECK-NEXT: [[COND2:%.*]] = icmp ugt i8* [[A2]], [[B:%.*]]
9-
; CHECK-NEXT: ret i1 [[COND2]]
7+
; CHECK-NEXT: [[COND1:%.*]] = icmp ne i8* [[A:%.*]], [[B:%.*]]
8+
; CHECK-NEXT: [[A2:%.*]] = getelementptr inbounds i8, i8* [[A]], i64 -1
9+
; CHECK-NEXT: [[COND2:%.*]] = icmp ugt i8* [[A2]], [[B]]
10+
; CHECK-NEXT: [[RES:%.*]] = select i1 [[COND1]], i1 [[COND2]], i1 false
11+
; CHECK-NEXT: ret i1 [[RES]]
1012
;
1113
%cond1 = icmp ne i8* %a, %b
1214
%a2 = getelementptr inbounds i8, i8* %a, i64 -1
@@ -18,9 +20,11 @@ define i1 @test1(i8* %a, i8* %b) {
1820
; The first comparison (a != b) should not be dropped
1921
define i1 @test2(i32 %a, i32 %b) {
2022
; CHECK-LABEL: @test2(
21-
; CHECK-NEXT: [[A2:%.*]] = add nuw i32 [[A:%.*]], 1
22-
; CHECK-NEXT: [[COND2:%.*]] = icmp ult i32 [[A2]], [[B:%.*]]
23-
; CHECK-NEXT: ret i1 [[COND2]]
23+
; CHECK-NEXT: [[COND1:%.*]] = icmp ne i32 [[A:%.*]], [[B:%.*]]
24+
; CHECK-NEXT: [[A2:%.*]] = add nuw i32 [[A]], 1
25+
; CHECK-NEXT: [[COND2:%.*]] = icmp ult i32 [[A2]], [[B]]
26+
; CHECK-NEXT: [[RES:%.*]] = select i1 [[COND1]], i1 [[COND2]], i1 false
27+
; CHECK-NEXT: ret i1 [[RES]]
2428
;
2529
%cond1 = icmp ne i32 %a, %b
2630
%a2 = add nuw i32 %a, 1

0 commit comments

Comments
 (0)