[InstCombine] Return nsw when convert shl to mul if its on the RHS

[goldsteinn]

This is follow up to #121633. We can keep the shl X, (BW-1) fold as
long the the shl is the RHS.

https://alive2.llvm.org/ce/z/P_vxSQ

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (goldsteinn)

Changes

This is follow up to #121633. We can keep the shl X, (BW-1) fold as
long the the shl is the RHS.

https://alive2.llvm.org/ce/z/P_vxSQ

---

Full diff: https://github.com/llvm/llvm-project/pull/121671.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (+3-3)
• (modified) llvm/test/Transforms/InstCombine/rem-mul-shl.ll (+1-3)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 97a765ecfb6bd5..3eda13ec7aeae3 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -2099,9 +2099,9 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
     return false;
   };
 
-  bool Op0PreserveNSW = true, Op1PreserveNSW = true;
+  bool Op0PreserveNSW = true, Unused;
   if (MatchShiftOrMulXC(Op0, X, Y, Op0PreserveNSW) &&
-      MatchShiftOrMulXC(Op1, X, Z, Op1PreserveNSW)) {
+      MatchShiftOrMulXC(Op1, X, Z, Unused)) {
     // pass
   } else if (MatchShiftCX(Op0, Y, X) && MatchShiftCX(Op1, Z, X)) {
     ShiftByX = true;
@@ -2137,7 +2137,7 @@ static Instruction *simplifyIRemMulShl(BinaryOperator &I,
   };
 
   OverflowingBinaryOperator *BO1 = cast<OverflowingBinaryOperator>(Op1);
-  bool BO1HasNSW = Op1PreserveNSW && BO1->hasNoSignedWrap();
+  bool BO1HasNSW = BO1->hasNoSignedWrap();
   bool BO1HasNUW = BO1->hasNoUnsignedWrap();
   bool BO1NoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
   // (rem (mul X, Y), (mul nuw/nsw X, Z))
diff --git a/llvm/test/Transforms/InstCombine/rem-mul-shl.ll b/llvm/test/Transforms/InstCombine/rem-mul-shl.ll
index 920497c07e3804..1db59052d70c8c 100644
--- a/llvm/test/Transforms/InstCombine/rem-mul-shl.ll
+++ b/llvm/test/Transforms/InstCombine/rem-mul-shl.ll
@@ -388,9 +388,7 @@ define i8 @srem_XY_XZ_with_CY_gt_CZ_drop_nsw(i8 noundef %X) {
 define i8 @srem_XY_XZ_with_CY_gt_CZ_drop_nsw_commuted(i8 noundef %X) {
 ; 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]]
+; CHECK-NEXT:    ret i8 [[BO0]]
 ;
   %BO0 = mul nsw i8 %X, 127
   %BO1 = shl nsw i8 %X, 7

[nikic]

No strong opinion, but I'd leave this alone. The current code is obvious (we're just converting shl to mul and determine whether the mul has the nsw flag), while this would need some explanation for why it's okay to keep the nsw in one case but not the other.

[goldsteinn]

No strong opinion, but I'd leave this alone. The current code is obvious (we're just converting shl to mul and determine whether the mul has the nsw flag), while this would need some explanation for why it's okay to keep the nsw in one case but not the other.

I'm less concerned w/ keeping the nsw than performing the fold, but either way I doubt this shows up on opt benchmark so agreed its not priority.