From 3275060fe83841f7a4e22d3690738fe1677cf12e Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Fri, 17 May 2019 15:52:49 +0000 Subject: [PATCH] [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus signbit check Summary: In D61918 i was looking at dropping it in DAGCombiner `visitShiftByConstant()`, but as @craig.topper pointed out, it was copied from here. That check claims that the transform is illegal otherwise. That isn't true: 1. For `ISD::ADD`, we only process `ISD::SHL` outer shift => sign bit does not matter https://rise4fun.com/Alive/K4A 2. For `ISD::AND`, there is no restriction on constants: https://rise4fun.com/Alive/Wy3 3. For `ISD::OR`, there is no restriction on constants: https://rise4fun.com/Alive/GOH 3. For `ISD::XOR`, there is no restriction on constants: https://rise4fun.com/Alive/ml6 So, why is it there then? As far as i can tell, it dates all the way back to original check-in rL7793. I think we should just drop it. Reviewers: spatel, craig.topper, efriedma, majnemer Reviewed By: spatel Subscribers: llvm-commits, craig.topper Tags: #llvm Differential Revision: https://reviews.llvm.org/D61938 llvm-svn: 361043 --- .../InstCombine/InstCombineShifts.cpp | 34 +++++-------------- .../InstCombine/pull-binop-through-shift.ll | 8 ++--- .../pull-conditional-binop-through-shift.ll | 18 +++++----- llvm/test/Transforms/InstCombine/trunc.ll | 8 ++--- 4 files changed, 25 insertions(+), 43 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp index fbbba4632d02..21f1ff56602b 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp @@ -312,35 +312,17 @@ static Value *getShiftedValue(Value *V, unsigned NumBits, bool isLeftShift, // If this is a bitwise operator or add with a constant RHS we might be able // to pull it through a shift. static bool canShiftBinOpWithConstantRHS(BinaryOperator &Shift, - BinaryOperator *BO, - const APInt &C) { - bool IsValid = true; // Valid only for And, Or Xor, - bool HighBitSet = false; // Transform ifhigh bit of constant set? - + BinaryOperator *BO) { switch (BO->getOpcode()) { - default: IsValid = false; break; // Do not perform transform! + default: + return false; // Do not perform transform! case Instruction::Add: - IsValid = Shift.getOpcode() == Instruction::Shl; - break; + return Shift.getOpcode() == Instruction::Shl; case Instruction::Or: case Instruction::Xor: - HighBitSet = false; - break; case Instruction::And: - HighBitSet = true; - break; + return true; } - - // If this is a signed shift right, and the high bit is modified - // by the logical operation, do not perform the transformation. - // The HighBitSet boolean indicates the value of the high bit of - // the constant which would cause it to be modified for this - // operation. - // - if (IsValid && Shift.getOpcode() == Instruction::AShr) - IsValid = C.isNegative() == HighBitSet; - - return IsValid; } Instruction *InstCombiner::FoldShiftByConstant(Value *Op0, Constant *Op1, @@ -507,7 +489,7 @@ Instruction *InstCombiner::FoldShiftByConstant(Value *Op0, Constant *Op1, // shift is the only use, we can pull it out of the shift. const APInt *Op0C; if (match(Op0BO->getOperand(1), m_APInt(Op0C))) { - if (canShiftBinOpWithConstantRHS(I, Op0BO, *Op0C)) { + if (canShiftBinOpWithConstantRHS(I, Op0BO)) { Constant *NewRHS = ConstantExpr::get(I.getOpcode(), cast(Op0BO->getOperand(1)), Op1); @@ -551,7 +533,7 @@ Instruction *InstCombiner::FoldShiftByConstant(Value *Op0, Constant *Op1, const APInt *C; if (!isa(FalseVal) && TBO->getOperand(0) == FalseVal && match(TBO->getOperand(1), m_APInt(C)) && - canShiftBinOpWithConstantRHS(I, TBO, *C)) { + canShiftBinOpWithConstantRHS(I, TBO)) { Constant *NewRHS = ConstantExpr::get(I.getOpcode(), cast(TBO->getOperand(1)), Op1); @@ -570,7 +552,7 @@ Instruction *InstCombiner::FoldShiftByConstant(Value *Op0, Constant *Op1, const APInt *C; if (!isa(TrueVal) && FBO->getOperand(0) == TrueVal && match(FBO->getOperand(1), m_APInt(C)) && - canShiftBinOpWithConstantRHS(I, FBO, *C)) { + canShiftBinOpWithConstantRHS(I, FBO)) { Constant *NewRHS = ConstantExpr::get(I.getOpcode(), cast(FBO->getOperand(1)), Op1); diff --git a/llvm/test/Transforms/InstCombine/pull-binop-through-shift.ll b/llvm/test/Transforms/InstCombine/pull-binop-through-shift.ll index 5892134f210e..b39ec80cd70b 100644 --- a/llvm/test/Transforms/InstCombine/pull-binop-through-shift.ll +++ b/llvm/test/Transforms/InstCombine/pull-binop-through-shift.ll @@ -198,8 +198,8 @@ define i32 @and_nosignbit_ashr(i32 %x) { define i32 @or_signbit_ashr(i32 %x) { ; CHECK-LABEL: @or_signbit_ashr( -; CHECK-NEXT: [[T0:%.*]] = or i32 [[X:%.*]], -65536 -; CHECK-NEXT: [[R:%.*]] = ashr i32 [[T0]], 8 +; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[R:%.*]] = or i32 [[TMP1]], -256 ; CHECK-NEXT: ret i32 [[R]] ; %t0 = or i32 %x, 4294901760 ; 0xFFFF0000 @@ -219,8 +219,8 @@ define i32 @or_nosignbit_ashr(i32 %x) { define i32 @xor_signbit_ashr(i32 %x) { ; CHECK-LABEL: @xor_signbit_ashr( -; CHECK-NEXT: [[T0:%.*]] = xor i32 [[X:%.*]], -65536 -; CHECK-NEXT: [[R:%.*]] = ashr i32 [[T0]], 8 +; CHECK-NEXT: [[T0:%.*]] = ashr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[R:%.*]] = xor i32 [[T0]], -256 ; CHECK-NEXT: ret i32 [[R]] ; %t0 = xor i32 %x, 4294901760 ; 0xFFFF0000 diff --git a/llvm/test/Transforms/InstCombine/pull-conditional-binop-through-shift.ll b/llvm/test/Transforms/InstCombine/pull-conditional-binop-through-shift.ll index 3bd78734e0bd..438fe58c2689 100644 --- a/llvm/test/Transforms/InstCombine/pull-conditional-binop-through-shift.ll +++ b/llvm/test/Transforms/InstCombine/pull-conditional-binop-through-shift.ll @@ -221,9 +221,9 @@ define i32 @and_signbit_select_ashr(i32 %x, i1 %cond) { } define i32 @and_nosignbit_select_ashr(i32 %x, i1 %cond) { ; CHECK-LABEL: @and_nosignbit_select_ashr( -; CHECK-NEXT: [[T0:%.*]] = and i32 [[X:%.*]], 2147418112 -; CHECK-NEXT: [[T1:%.*]] = select i1 [[COND:%.*]], i32 [[T0]], i32 [[X]] -; CHECK-NEXT: [[R:%.*]] = ashr i32 [[T1]], 8 +; CHECK-NEXT: [[TMP1:%.*]] = ashr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[TMP2:%.*]] = and i32 [[TMP1]], 8388352 +; CHECK-NEXT: [[R:%.*]] = select i1 [[COND:%.*]], i32 [[TMP2]], i32 [[TMP1]] ; CHECK-NEXT: ret i32 [[R]] ; %t0 = and i32 %x, 2147418112 ; 0x7FFF0000 @@ -234,9 +234,9 @@ define i32 @and_nosignbit_select_ashr(i32 %x, i1 %cond) { define i32 @or_signbit_select_ashr(i32 %x, i1 %cond) { ; CHECK-LABEL: @or_signbit_select_ashr( -; CHECK-NEXT: [[T0:%.*]] = or i32 [[X:%.*]], -65536 -; CHECK-NEXT: [[T1:%.*]] = select i1 [[COND:%.*]], i32 [[T0]], i32 [[X]] -; CHECK-NEXT: [[R:%.*]] = ashr i32 [[T1]], 8 +; CHECK-NEXT: [[TMP1:%.*]] = ashr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[TMP2:%.*]] = or i32 [[TMP1]], -256 +; CHECK-NEXT: [[R:%.*]] = select i1 [[COND:%.*]], i32 [[TMP2]], i32 [[TMP1]] ; CHECK-NEXT: ret i32 [[R]] ; %t0 = or i32 %x, 4294901760 ; 0xFFFF0000 @@ -259,9 +259,9 @@ define i32 @or_nosignbit_select_ashr(i32 %x, i1 %cond) { define i32 @xor_signbit_select_ashr(i32 %x, i1 %cond) { ; CHECK-LABEL: @xor_signbit_select_ashr( -; CHECK-NEXT: [[T0:%.*]] = xor i32 [[X:%.*]], -65536 -; CHECK-NEXT: [[T1:%.*]] = select i1 [[COND:%.*]], i32 [[T0]], i32 [[X]] -; CHECK-NEXT: [[R:%.*]] = ashr i32 [[T1]], 8 +; CHECK-NEXT: [[TMP1:%.*]] = ashr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[TMP2:%.*]] = xor i32 [[TMP1]], -256 +; CHECK-NEXT: [[R:%.*]] = select i1 [[COND:%.*]], i32 [[TMP2]], i32 [[TMP1]] ; CHECK-NEXT: ret i32 [[R]] ; %t0 = xor i32 %x, 4294901760 ; 0xFFFF0000 diff --git a/llvm/test/Transforms/InstCombine/trunc.ll b/llvm/test/Transforms/InstCombine/trunc.ll index bff2fc3770fe..750f62433664 100644 --- a/llvm/test/Transforms/InstCombine/trunc.ll +++ b/llvm/test/Transforms/InstCombine/trunc.ll @@ -125,8 +125,8 @@ define i16 @ashr_mul(i8 %X, i8 %Y) { define i32 @trunc_ashr(i32 %X) { ; CHECK-LABEL: @trunc_ashr( -; CHECK-NEXT: [[B:%.*]] = or i32 [[X:%.*]], -2147483648 -; CHECK-NEXT: [[C:%.*]] = ashr i32 [[B]], 8 +; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[X:%.*]], 8 +; CHECK-NEXT: [[C:%.*]] = or i32 [[TMP1]], -8388608 ; CHECK-NEXT: ret i32 [[C]] ; %A = zext i32 %X to i36 @@ -138,8 +138,8 @@ define i32 @trunc_ashr(i32 %X) { define <2 x i32> @trunc_ashr_vec(<2 x i32> %X) { ; CHECK-LABEL: @trunc_ashr_vec( -; CHECK-NEXT: [[B:%.*]] = or <2 x i32> [[X:%.*]], -; CHECK-NEXT: [[C:%.*]] = ashr <2 x i32> [[B]], +; CHECK-NEXT: [[TMP1:%.*]] = lshr <2 x i32> [[X:%.*]], +; CHECK-NEXT: [[C:%.*]] = or <2 x i32> [[TMP1]], ; CHECK-NEXT: ret <2 x i32> [[C]] ; %A = zext <2 x i32> %X to <2 x i36> -- 2.34.1