[PATCH] D143014: [InstCombine] Add combines for `(urem/srem (mul/shl X, Y), (mul/shl X, Z))`

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added a comment.

@goldstein.w.n Please update the commit message/description to properly reflect the patch after it was split (e.g. references to one use removed, an example of the Y >= Z transform etc)



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1742
+  }
+  bool IsSigned = I.getOpcode() == Instruction::SRem;
+
----------------
MattDevereau wrote:
> I think `IsSRem` is more descriptive and clearer to read later on. Now that we've removed the nested if statements we can move this assignment directly before its first use.
This comment isn't done, IsSRem is still defined far before it is first used


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1743-1747
+
+  bool NSW0 = BO0->hasNoSignedWrap();
+  bool NSW1 = BO1->hasNoSignedWrap();
+  bool NUW0 = BO0->hasNoUnsignedWrap();
+  bool NUW1 = BO1->hasNoUnsignedWrap();
----------------
MattDevereau wrote:
> For these names maybe call them something like `BO0HasNSW`, `BO1HasNSW` to make it read a bit more literally and lessen the reader's memory overhead of what the 0 in NSW0 represents. We can move these assignments directly before their first use now.
This comment isn't done, these variables are still defined far before they are used.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1688
+
+  // Variety of transform for (urem/srem (mul/shl X, Y), (mul/shl X, Z))
+  Value *A, *B, *C, *D;
----------------
A Variety of transforms. This comment can be moved above the function.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1709-1718
+  if (!X)
+    return nullptr;
+
+  // BO0 = X * Y
+  auto *BO0 = dyn_cast<BinaryOperator>(Op0);
+  // BO1 = X * Z
+  auto *BO1 = dyn_cast<BinaryOperator>(Op1);
----------------
```
  auto *BO0 = dyn_cast<BinaryOperator>(Op0);
  auto *BO1 = dyn_cast<BinaryOperator>(Op1);
  if (!X || !BO0 || !BO1)
    return nullptr;
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1721
+  // If X is constant 1, then we avoid both in the mul and shl case.
+  auto *CX = dyn_cast<Constant>(X);
+  if (CX && CX->isOneValue())
----------------
The `*` here is not needed 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1725-1738
+  auto *ConstY = dyn_cast<ConstantInt>(Y);
+  auto *ConstZ = dyn_cast<ConstantInt>(Z);
+  if (I.getType()->isVectorTy()) {
+    auto *VConstY = dyn_cast<Constant>(Y);
+    auto *VConstZ = dyn_cast<Constant>(Z);
+    if (VConstY && VConstZ) {
+      VConstY = VConstY->getSplatValue();
----------------
We can do

```
  ConstantInt *ConstY, *ConstZ;
  Constant *CY, *CZ;
  if (!I.getType()->isVectorTy() || !match(Y, m_Constant(CY)) ||
      !match(CY->getSplatValue(), m_ConstantInt(ConstY)) ||
      !match(Z, m_Constant(CZ)) ||
      !match(CZ->getSplatValue(), m_ConstantInt(ConstZ)))
    if (!match(Y, m_ConstantInt(ConstY)) || !match(Z, m_ConstantInt(ConstZ)))
      return nullptr;
```

Which saves us a few lines, and this also lets us remove the later check for

```
  if (!ConstY || !ConstZ)
    return nullptr;
```


================
Comment at: llvm/test/Transforms/InstCombine/rem-mul-shl.ll:32-41
 define i8 @urem_XY_XZ_with_CY_rem_CZ_eq_0(i8 %X) {
 ; CHECK-LABEL: @urem_XY_XZ_with_CY_rem_CZ_eq_0(
-; CHECK-NEXT:    [[BO0:%.*]] = mul nuw i8 [[X:%.*]], 15
-; CHECK-NEXT:    [[BO1:%.*]] = mul i8 [[X]], 5
-; CHECK-NEXT:    [[R:%.*]] = urem i8 [[BO0]], [[BO1]]
-; CHECK-NEXT:    ret i8 [[R]]
+; CHECK-NEXT:    ret i8 0
 ;
   %BO0 = mul nuw i8 %X, 15
   %BO1 = mul i8 %X, 5
   %r = urem i8 %BO0, %BO1
----------------
Please add a scalable version of this test:

```
define <vscale x 16 x i8> @urem_XY_XZ_with_CY_rem_CZ_eq_0_scalable(<vscale x 16 x i8> %X) {
; CHECK-LABEL: @urem_XY_XZ_with_CY_rem_CZ_eq_0_scalable(
; CHECK-NEXT:    ret <vscale x 16 x i8> zeroinitializer
;
  %BO0 = mul nuw <vscale x 16 x i8> %X, shufflevector (<vscale x 16 x i8> insertelement (<vscale x 16 x i8> poison, i8 15, i64 0), <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer)
  %BO1 = mul <vscale x 16 x i8> %X, shufflevector (<vscale x 16 x i8> insertelement (<vscale x 16 x i8> poison, i8 5, i64 0), <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer)
  %r = urem <vscale x 16 x i8> %BO0, %BO1
  ret <vscale x 16 x i8> %r
}
```


================
Comment at: llvm/test/Transforms/InstCombine/rem-mul-shl.ll:218-228
 ;; Signed Verions
 define i8 @srem_XY_XZ_with_CY_rem_CZ_eq_0(i8 %X) {
 ; CHECK-LABEL: @srem_XY_XZ_with_CY_rem_CZ_eq_0(
-; CHECK-NEXT:    [[BO0:%.*]] = mul nsw i8 [[X:%.*]], 9
-; CHECK-NEXT:    [[BO1:%.*]] = mul i8 [[X]], 3
-; CHECK-NEXT:    [[R:%.*]] = srem i8 [[BO0]], [[BO1]]
-; CHECK-NEXT:    ret i8 [[R]]
+; CHECK-NEXT:    ret i8 0
 ;
   %BO0 = mul nsw i8 %X, 9
   %BO1 = mul i8 %X, 3
----------------
Please add a scalable version of this test:

```
define <vscale x 16 x i8> @srem_XY_XZ_with_CY_rem_CZ_eq_0_scalable(<vscale x 16 x i8> %X) {
; CHECK-LABEL: @srem_XY_XZ_with_CY_rem_CZ_eq_0_scalable(
; CHECK-NEXT:    ret <vscale x 16 x i8> zeroinitializer
;
  %BO0 = mul nsw <vscale x 16 x i8> %X, shufflevector (<vscale x 16 x i8> insertelement (<vscale x 16 x i8> poison, i8 15, i64 0), <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer)
  %BO1 = mul <vscale x 16 x i8> %X, shufflevector (<vscale x 16 x i8> insertelement (<vscale x 16 x i8> poison, i8 5, i64 0), <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer)
  %r = srem <vscale x 16 x i8> %BO0, %BO1
  ret <vscale x 16 x i8> %r
}

```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143014/new/

https://reviews.llvm.org/D143014