[PATCH] D114279: [InstSimplify] Fold X {lshr,udiv} C <u X --> true for nonzero X, non-identity C

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2938
 
-  // x >> y <=u x
-  // x udiv y <=u x.
+  // x >> y <=u x --> true.
+  // x >> y >u  x --> false.
----------------
Nit: here and below, we're using "u" or "udiv" to make the unsigned requirements explicit, but we're using the ambiguous ">>" for the shift op. 
Better to make that ">>u" or spell out "lshr"?


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2952
+  // x udiv C <u x --> true for C > 1.
+  // TODO: allow non-constant shift amount/divisor
+  const APInt *C;
----------------
erikdesjardins wrote:
> spatel wrote:
> > Is that TODO feasible? The lshr half could use isKnownNonZero, but how would we deal with ">1" for udiv?
> I think something like
> 
>   KnownBits Known = computeKnownBits(Divisor, ...);
>   if (KnownBits::ne(Known, KnownBits::makeConstant(1)))
> 
> would technically work.
> 
> I don't know if this would help much in practice though; I can remove "divisor" from the comment if you prefer.
It's fine to leave it. I was just wondering how we'd do that. :)
There's also a question of whether it's worth the compile-time, but these patterns are probably rare enough that it is ok (assuming it would fire on real code enough times to make a difference).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114279