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

This patch is quite difficult to review in it's current state. There are over 20 if statements with various levels of nesting in one function, with the largest if statement spanning 150 lines. I think some of these cases could be handled better with helper functions, where you could pass the binary operators through and deduce if the flags and operands are valid for a combine from within those functions. It's very difficult to keep track of which variables are which when they are named A, Y, BO1, CY, CZ, AY, AZ in combination with the long and layered if statements with verbose comments.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1804-1809
+        // (urem (mul nuw X, Y), (mul X, Z))
+        //      if (urem Y, Z) == 0
+        //          -> 0
+        // (srem (mul nsw X, Y), (mul X, Z))
+        //      if (srem Y, Z) == 0
+        //          -> 0
----------------
This comment spans several lines and both variants describe basically the same thing. I think something like

```
// (rem (mul nuw/nsw X, Y), (mul X, Z))
//      if (rem Y, Z) == 0
//          -> 0
```
is probably enough.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1812-1818
+
+        // (urem (mul X, Y), (mul nuw X, Z))
+        //      if (urem Y, Z) == Y
+        //          -> (mul nuw X, Y)
+        // (srem (mul X, Y), (mul nsw X, Z))
+        //      if (srem Y, Z) == Y
+        //          -> (mul nsw X, Y)
----------------
You can simplify this comment to one case


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1842-1844
+          // We can add nuw if remainder is not signed or we have nuw on Op0
+          if (!IsSigned || NUW0)
+            BO->setHasNoUnsignedWrap();
----------------
I think the code is self explanatory and describes this better than the comment


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1849-1860
+      // If Op0/Op1 only have one use (here), then we will be saving several
+      // instructions at no real cost so proceed. Otherwise we will either being
+      // going even (either Op0 or Op1 has one use but not both), or adding an
+      // additional instruction (neither have one use) so only do so if Y and Z
+      // are constant. As well, don't replace for urem Op0 is shl and Op1 is mul
+      // or for srem if either Op0/Op1 are shl. In the urem case we go even, and
+      // the srem case slightly worse, so there is no real benefit.
----------------
This comment is very verbose, its longer than the if statement and difficult to understand. If there are exceptions for particular cases then you can probably describe them next to the cases. This may also make more sense to explain in a commit message or next to specific tests.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1866-1869
+        // (urem (mul nuw X, Y), (mul nuw X, Z))
+        //        -> (mul nuw X, (urem Y, Z))
+        // (srem (mul nsw X, Y), (mul nsw nuw X, Z)
+        //        -> (mul nsw X, (srem Y, Z))
----------------
Comment can be simplified to one case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014