[PATCH] D61938: [InstCombine] canShiftBinOpWithConstantRHS(): drop bogus(?) signbit check
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 15 03:58:14 PDT 2019
lebedev.ri created this revision.
lebedev.ri added reviewers: spatel, craig.topper.
lebedev.ri added a project: LLVM.
In D61918 <https://reviews.llvm.org/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
4. 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 rL92709 <https://reviews.llvm.org/rL92709>.
I think we should just drop it.
Repository:
rL LLVM
https://reviews.llvm.org/D61938
Files:
lib/Transforms/InstCombine/InstCombineShifts.cpp
test/Transforms/InstCombine/trunc.ll
Index: test/Transforms/InstCombine/trunc.ll
===================================================================
--- test/Transforms/InstCombine/trunc.ll
+++ test/Transforms/InstCombine/trunc.ll
@@ -125,8 +125,8 @@
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 <2 x i32> @trunc_ashr_vec(<2 x i32> %X) {
; CHECK-LABEL: @trunc_ashr_vec(
-; CHECK-NEXT: [[B:%.*]] = or <2 x i32> [[X:%.*]], <i32 -2147483648, i32 -2147483648>
-; CHECK-NEXT: [[C:%.*]] = ashr <2 x i32> [[B]], <i32 8, i32 8>
+; CHECK-NEXT: [[TMP1:%.*]] = lshr <2 x i32> [[X:%.*]], <i32 8, i32 8>
+; CHECK-NEXT: [[C:%.*]] = or <2 x i32> [[TMP1]], <i32 -8388608, i32 -8388608>
; CHECK-NEXT: ret <2 x i32> [[C]]
;
%A = zext <2 x i32> %X to <2 x i36>
Index: lib/Transforms/InstCombine/InstCombineShifts.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -312,35 +312,20 @@
// 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;
+ break; // Do not perform transform!
case Instruction::Add:
- IsValid = Shift.getOpcode() == Instruction::Shl;
+ return Shift.getOpcode() == Instruction::Shl;
break;
case Instruction::Or:
case Instruction::Xor:
- HighBitSet = false;
- break;
case Instruction::And:
- HighBitSet = true;
+ return true;
break;
}
-
- // 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 +492,7 @@
// 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<Constant>(Op0BO->getOperand(1)), Op1);
@@ -551,7 +536,7 @@
const APInt *C;
if (!isa<Constant>(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<Constant>(TBO->getOperand(1)), Op1);
@@ -570,7 +555,7 @@
const APInt *C;
if (!isa<Constant>(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<Constant>(FBO->getOperand(1)), Op1);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61938.199576.patch
Type: text/x-patch
Size: 4050 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190515/1d8f85af/attachment.bin>
More information about the llvm-commits
mailing list