[PATCH] D141990: [InstSimplify] Add transform ctpop(X) -> 1 iff X is non-zero power of 2

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:5886
                                      const SimplifyQuery &Q) {
+  Value * Op0 = Call->getArgOperand(0);
   // Idempotent functions return the same result when called repeatedly.
----------------
nikic wrote:
> Any particular reason for this change? It doesn't look like you make use of it (note that Call->getType() == Op0->getType() below).
> Any particular reason for this change? It doesn't look like you make use of it (note that Call->getType() == Op0->getType() below).

Didn't know that was guranteed, fixed.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:5923-5924
   case Intrinsic::ctpop: {
+    // ctpop(X) -> 1 iff X is non-zero power of 2. Maybe better also to just
+    // return icmp X, 0 if its known power of 2 but not known non-zero.
+    if (isKnownToBeAPowerOfTwo(Op0, Q.DL, /*OrZero*/ false, 0, Q.AC, Q.CxtI,
----------------
spatel wrote:
> Remove the "Maybe better..." statement because we can't do that in InstSimplify (can't create new instructions here). But that should happen in InstCombine if it's not already done there.
> Remove the "Maybe better..." statement because we can't do that in InstSimplify (can't create new instructions here). But that should happen in InstCombine if it's not already done there.

Is this change needed at all then? Would just knownpow2 -> icmp ne 0 in `InstCombine` be better?

But can do regardless. One thing, assuming we keep this AND add `InstCombine` change. Should I duplicate the test file or just add `instcombine` as a second pass in the current tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141990