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

I think not implementing several of my suggestions because of a future patch is a mistake. I don't think they're nits and have some obvious benefits for readability and control flow, and I'm of the opinion that leaving code that anticipates future work that may or may not even land or be reverted is not ideal, so I'll leave this for someone else to approve.



================
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())
----------------
goldstein.w.n wrote:
> MattDevereau wrote:
> > The `*` here is not needed 
> I prefer to keep `*` when its a pointer, think its clearer.
nit: If you don't use auto to hide away small details like this for short lived variables then I don't see much reason to use it at all. If you combine the below block into match statements the fact that these are pointers becomes an unimportant detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014