Skip to content

Commit b2b41bc

Browse files
LebedevRIzmodem
authored andcommitted
[InstCombine] foldShiftIntoShiftInAnotherHandOfAndInICmp(): fix miscompile (PR44802)
Much like with reassociateShiftAmtsOfTwoSameDirectionShifts(), as input, we have the following pattern: icmp eq/ne (and ((x shift Q), (y oppositeshift K))), 0 We want to rewrite that as: icmp eq/ne (and (x shift (Q+K)), y), 0 iff (Q+K) u< bitwidth(x) While we know that originally (Q+K) would not overflow (because 2 * (N-1) u<= iN -1), we may have looked past extensions of shift amounts. so it may now overflow in smaller bitwidth. To ensure that does not happen, we need to ensure that the total maximal shift amount is still representable in that smaller bitwidth. If the overflow would happen, (Q+K) u< bitwidth(x) check would be bogus. https://bugs.llvm.org/show_bug.cgi?id=44802 (cherry picked from commit 2855c8f)
1 parent ac293ed commit b2b41bc

File tree

2 files changed

+26
-5
lines changed

2 files changed

+26
-5
lines changed

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3494,7 +3494,8 @@ foldShiftIntoShiftInAnotherHandOfAndInICmp(ICmpInst &I, const SimplifyQuery SQ,
34943494
Instruction *NarrowestShift = XShift;
34953495

34963496
Type *WidestTy = WidestShift->getType();
3497-
assert(NarrowestShift->getType() == I.getOperand(0)->getType() &&
3497+
Type *NarrowestTy = NarrowestShift->getType();
3498+
assert(NarrowestTy == I.getOperand(0)->getType() &&
34983499
"We did not look past any shifts while matching XShift though.");
34993500
bool HadTrunc = WidestTy != I.getOperand(0)->getType();
35003501

@@ -3533,6 +3534,23 @@ foldShiftIntoShiftInAnotherHandOfAndInICmp(ICmpInst &I, const SimplifyQuery SQ,
35333534
if (XShAmt->getType() != YShAmt->getType())
35343535
return nullptr;
35353536

3537+
// As input, we have the following pattern:
3538+
// icmp eq/ne (and ((x shift Q), (y oppositeshift K))), 0
3539+
// We want to rewrite that as:
3540+
// icmp eq/ne (and (x shift (Q+K)), y), 0 iff (Q+K) u< bitwidth(x)
3541+
// While we know that originally (Q+K) would not overflow
3542+
// (because 2 * (N-1) u<= iN -1), we have looked past extensions of
3543+
// shift amounts. so it may now overflow in smaller bitwidth.
3544+
// To ensure that does not happen, we need to ensure that the total maximal
3545+
// shift amount is still representable in that smaller bit width.
3546+
unsigned MaximalPossibleTotalShiftAmount =
3547+
(WidestTy->getScalarSizeInBits() - 1) +
3548+
(NarrowestTy->getScalarSizeInBits() - 1);
3549+
APInt MaximalRepresentableShiftAmount =
3550+
APInt::getAllOnesValue(XShAmt->getType()->getScalarSizeInBits());
3551+
if (MaximalRepresentableShiftAmount.ult(MaximalPossibleTotalShiftAmount))
3552+
return nullptr;
3553+
35363554
// Can we fold (XShAmt+YShAmt) ?
35373555
auto *NewShAmt = dyn_cast_or_null<Constant>(
35383556
SimplifyAddInst(XShAmt, YShAmt, /*isNSW=*/false,

llvm/test/Transforms/InstCombine/shift-amount-reassociation-in-bittest.ll

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -688,13 +688,16 @@ entry:
688688
ret i1 %tobool
689689
}
690690

691-
; FIXME: this is a miscompile. We should not transform this.
692691
; See https://bugs.llvm.org/show_bug.cgi?id=44802
693692
define i1 @pr44802(i3 %a, i3 %x, i3 %y) {
694693
; CHECK-LABEL: @pr44802(
695-
; CHECK-NEXT: [[TMP1:%.*]] = and i3 [[X:%.*]], [[Y:%.*]]
696-
; CHECK-NEXT: [[TMP2:%.*]] = icmp ne i3 [[TMP1]], 0
697-
; CHECK-NEXT: ret i1 [[TMP2]]
694+
; CHECK-NEXT: [[T0:%.*]] = icmp ne i3 [[A:%.*]], 0
695+
; CHECK-NEXT: [[T1:%.*]] = zext i1 [[T0]] to i3
696+
; CHECK-NEXT: [[T2:%.*]] = lshr i3 [[X:%.*]], [[T1]]
697+
; CHECK-NEXT: [[T3:%.*]] = shl i3 [[Y:%.*]], [[T1]]
698+
; CHECK-NEXT: [[T4:%.*]] = and i3 [[T2]], [[T3]]
699+
; CHECK-NEXT: [[T5:%.*]] = icmp ne i3 [[T4]], 0
700+
; CHECK-NEXT: ret i1 [[T5]]
698701
;
699702
%t0 = icmp ne i3 %a, 0
700703
%t1 = zext i1 %t0 to i3

0 commit comments

Comments
 (0)