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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen requested changes to this revision.
sdesmalen added a comment.
This revision now requires changes to proceed.

Hi @goldstein.w.n, I'm still going through the patch but already found that some code paths are currently untested. I am requesting changes to avoid you from landing it.
In general my preference would be to limit the cases your code handles, rather than adding more tests for more possible combinations of shifts/muls. That makes the code-changes easier to review and helps identify the test-coverage for the different code-paths you've added.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1695-1696
+  Value *X = nullptr, *Y, *Z;
+  // Do this by hand as opposed to using m_Specific because either A/B (or
+  // C/D) can be our X.
+  if (A == C || A == D) {
----------------
Is that true? `shl` is not commutative, i.e. `(a * b) == (b * a)`, but `(a << b) != (b << a)` 

Also, this patch is already quite complicated, I would prefer to keep it simple at first and only match: `urem/srem (mul/shl X, Y), (mul/shl X, Z)`
where 'X' is matched with m_Specific. Then later patches can maybe relax those conditions further.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1697
+  // C/D) can be our X.
+  if (A == C || A == D) {
+    X = A;
----------------
When I comment this part of the condition out, all tests still pass.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1701
+    Z = A == C ? D : C;
+  } else if (B == C || B == D) {
+    X = B;
----------------
When I comment this part of the condition out, all tests still pass.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1701
+    Z = A == C ? D : C;
+  } else if (B == C || B == D) {
+    X = B;
----------------
sdesmalen wrote:
> When I comment this part of the condition out, all tests still pass.
When I comment this part of the condition out, all tests still pass


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1716-1718
+  auto CX = dyn_cast<Constant>(X);
+  if (CX && CX->isOneValue())
+    return nullptr;
----------------
When I comment this code out, all tests still pass.

Can this case ever happen? (I would have expected InstCombine/InstSimplify to have folded that case away already)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014