[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.

The commit message needs cleaning up. There are references to cases based on having a single uses which are incorrectly left over from splitting up the patch. The alive tests would be easier to follow if each case contained strictly its necessary components instead of commenting out several cases.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1696-1697
+
+  Value *X, *Y, *Z;
+  X = nullptr;
+  // Do this by hand as opposed to using m_Specific because either A/B (or
----------------
Value *X = nullptr, *Y, *Z;


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1714
+  // BO0 = X * Y
+  BinaryOperator *BO0 = dyn_cast<BinaryOperator>(Op0);
+  // BO1 = X * Z
----------------
Use auto here since the type name is on the RHS of the assignment. This goes for all of the `dyn_cast` assignments below


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1726
+
+  Type *Ty = I.getType();
+
----------------
I don't think this variable gives us much benefit over simply `I.getType()`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1742
+  }
+  bool IsSigned = I.getOpcode() == Instruction::SRem;
+
----------------
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.


================
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();
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1749
+
+  // Try to handle constant cases first.
+  if (!ConstY || !ConstZ)
----------------
Comment can be removed since we're only handling the constant cases in this patch, and the code this is describing is self explanatory.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1775
+  if (RemYZ == APIntY && (IsSigned ? NSW1 : NUW1)) {
+    // We are returning Op0 essentially but we can also add no wrap flags.
+    BinaryOperator *BO =
----------------
This comment is noise as its just a verbal description of the one above


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1778-1779
+        BinaryOperator::CreateMul(X, ConstantInt::get(Ty, APIntY));
+    // We can add nsw/nuw if remainder op is signed/unsigned, also we
+    // can copy any overflow flags from Op0.
+    if (IsSigned || NSW0)
----------------
You can just keep `'// Copy any overflow flags from Op0`. The flag propagation is described above this if block and by the tests.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1790
+  //          -> (mul {nuw} nsw X, (rem Y, Z))
+  // NB: (rem Y, Z) is a constant.
+  if (APIntY.uge(APIntZ) && (IsSigned ? (NSW0 && NSW1) : NUW0)) {
----------------
This comment can be removed since we already know rem(Y, Z) is a constant as we've previously bailed out of the function with

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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1795
+    BO->setHasNoSignedWrap();
+    if (!IsSigned || NUW0)
+      BO->setHasNoUnsignedWrap();
----------------
This can be simplified to NUW0 since the top level if statement makes it impossible for NUW0 to be false if `IsSigned == false` at this point


================
Comment at: llvm/test/Transforms/InstCombine/urem-mul.ll:1
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
----------------
This file is called urem-mul.ll but should be called rem-mul.ll or srem-urem-mul.ll since it has both urem and srem


================
Comment at: llvm/test/Transforms/InstCombine/urem-mul.ll:41
 }
 
 define i8 @urem_XY_XZ_with_CY_rem_CZ_eq_0_fail_missing_flag(i8 %X) {
----------------
It would be good to have a positive test case where both sides of the rem are shl binops


================
Comment at: llvm/test/Transforms/InstCombine/urem-mul.ll:103
 ; CHECK-LABEL: @urem_CY_CZ_is_mul_X_RemYZ(
-; CHECK-NEXT:    [[BO0:%.*]] = mul nuw i8 [[X:%.*]], 21
-; CHECK-NEXT:    [[BO1:%.*]] = mul i8 [[X]], 6
-; CHECK-NEXT:    [[R:%.*]] = urem i8 [[BO0]], [[BO1]]
+; CHECK-NEXT:    [[R:%.*]] = mul nuw nsw i8 [[X:%.*]], 3
 ; CHECK-NEXT:    ret i8 [[R]]
----------------
Is it correct to generate a nsw flag for this case? Running this case through alive (https://alive2.llvm.org/ce/z/j9NY-S) I get a timeout unless I disable undef inputs, whereas just generating nuw accepts the transform (https://alive2.llvm.org/ce/z/332-Em)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014