Skip to content

[InstCombine] Drop NSW when converting shl X, BW - 1 back into mul #121633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 4, 2025
Merged

[InstCombine] Drop NSW when converting shl X, BW - 1 back into mul #121633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a773c26
[InstCombine] Add pre-commit tests. NFC.
dtcxzyw Jan 4, 2025
eed542e
[InstCombine] Drop NSW when converting `shl X, BW - 1` back into mul
dtcxzyw Jan 4, 2025
138b4ec
[InstCombine] Add commuted tests. NFC.
dtcxzyw Jan 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2066,14 +2066,18 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
bool ShiftByX = false;

// If V is not nullptr, it will be matched using m_Specific.
auto MatchShiftOrMulXC = [](Value *Op, Value *&V, APInt &C) -> bool {
auto MatchShiftOrMulXC = [](Value *Op, Value *&V, APInt &C,
bool &PreserveNSW) -> bool {
const APInt *Tmp = nullptr;
if ((!V && match(Op, m_Mul(m_Value(V), m_APInt(Tmp)))) ||
(V && match(Op, m_Mul(m_Specific(V), m_APInt(Tmp)))))
C = *Tmp;
else if ((!V && match(Op, m_Shl(m_Value(V), m_APInt(Tmp)))) ||
(V && match(Op, m_Shl(m_Specific(V), m_APInt(Tmp)))))
(V && match(Op, m_Shl(m_Specific(V), m_APInt(Tmp))))) {
C = APInt(Tmp->getBitWidth(), 1) << *Tmp;
// We cannot preserve NSW when shifting by BW - 1.
PreserveNSW = Tmp->ult(Tmp->getBitWidth() - 1);
}
if (Tmp != nullptr)
return true;

Expand All @@ -2095,7 +2099,9 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
return false;
};

if (MatchShiftOrMulXC(Op0, X, Y) && MatchShiftOrMulXC(Op1, X, Z)) {
bool Op0PreserveNSW = true, Op1PreserveNSW = true;
if (MatchShiftOrMulXC(Op0, X, Y, Op0PreserveNSW) &&
MatchShiftOrMulXC(Op1, X, Z, Op1PreserveNSW)) {
// pass
} else if (MatchShiftCX(Op0, Y, X) && MatchShiftCX(Op1, Z, X)) {
ShiftByX = true;
Expand All @@ -2108,7 +2114,7 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
OverflowingBinaryOperator *BO0 = cast<OverflowingBinaryOperator>(Op0);
// TODO: We may be able to deduce more about nsw/nuw of BO0/BO1 based on Y >=
// Z or Z >= Y.
bool BO0HasNSW = BO0->hasNoSignedWrap();
bool BO0HasNSW = Op0PreserveNSW && BO0->hasNoSignedWrap();
bool BO0HasNUW = BO0->hasNoUnsignedWrap();
bool BO0NoWrap = IsSRem ? BO0HasNSW : BO0HasNUW;

Expand All @@ -2131,7 +2137,7 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
};

OverflowingBinaryOperator *BO1 = cast<OverflowingBinaryOperator>(Op1);
bool BO1HasNSW = BO1->hasNoSignedWrap();
bool BO1HasNSW = Op1PreserveNSW && BO1->hasNoSignedWrap();
bool BO1HasNUW = BO1->hasNoUnsignedWrap();
bool BO1NoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
// (rem (mul X, Y), (mul nuw/nsw X, Z))
Expand Down
26 changes: 26 additions & 0 deletions llvm/test/Transforms/InstCombine/rem-mul-shl.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,32 @@ define <2 x i8> @srem_XY_XZ_with_CY_gt_CZ_no_nuw_out(<2 x i8> %X) {
ret <2 x i8> %r
}

define i8 @srem_XY_XZ_with_CY_gt_CZ_drop_nsw(i8 noundef %X) {
; CHECK-LABEL: @srem_XY_XZ_with_CY_gt_CZ_drop_nsw(
; CHECK-NEXT: [[BO0:%.*]] = mul nsw i8 [[X:%.*]], 127
; CHECK-NEXT: [[BO1:%.*]] = shl nsw i8 [[X]], 7
; CHECK-NEXT: [[R:%.*]] = srem i8 [[BO1]], [[BO0]]
; CHECK-NEXT: ret i8 [[R]]
;
%BO0 = mul nsw i8 %X, 127
%BO1 = shl nsw i8 %X, 7
%r = srem i8 %BO1, %BO0
ret i8 %r
}

define i8 @srem_XY_XZ_with_CY_gt_CZ_drop_nsw_commuted(i8 noundef %X) {
Copy link
Member Author

@dtcxzyw dtcxzyw Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original transform is valid: https://alive2.llvm.org/ce/z/htzvgR
But I don't know how to generalize this.

Copy link
Contributor

@goldsteinn goldsteinn Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think its okay if the shl is the rhs of the rem: https://alive2.llvm.org/ce/z/SoTDqo

Copy link
Contributor

@goldsteinn goldsteinn Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think you can drop Op1PreserveNSW

; CHECK-LABEL: @srem_XY_XZ_with_CY_gt_CZ_drop_nsw_commuted(
; CHECK-NEXT: [[BO0:%.*]] = mul nsw i8 [[X:%.*]], 127
; CHECK-NEXT: [[BO1:%.*]] = shl nsw i8 [[X]], 7
; CHECK-NEXT: [[R:%.*]] = srem i8 [[BO0]], [[BO1]]
; CHECK-NEXT: ret i8 [[R]]
;
%BO0 = mul nsw i8 %X, 127
%BO1 = shl nsw i8 %X, 7
%r = srem i8 %BO0, %BO1
ret i8 %r
}

define i8 @srem_XY_XZ_with_CY_gt_CZ_fail_missing_flag1(i8 %X) {
; CHECK-LABEL: @srem_XY_XZ_with_CY_gt_CZ_fail_missing_flag1(
; CHECK-NEXT: [[BO0:%.*]] = mul nuw nsw i8 [[X:%.*]], 10
Expand Down
Loading