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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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,
----------------
goldstein.w.n wrote:
> spatel wrote:
> > goldstein.w.n wrote:
> > > 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?
> > It's a trade-off, and I'm not sure which way is better in this case. It's nice to make InstSimplify stronger because other passes (like EarlyCSE and GVN) use this as an analysis. But you're correct that it's also good to consolidate related folds. I don't think there's a real compile-time concern either way since ctpop should be too rare to make any difference to overall compile-time.
> > 
> > If you want to add the fold for known-pow2-or-zero to instcombine, add those tests to InstCombine rather than here. There shouldn't be a need to duplicate the ones that are already handled here. The regression tests are meant to be minimal, so we don't want to run multiple/different passes here.
> > It's a trade-off, and I'm not sure which way is better in this case. It's nice to make InstSimplify stronger because other passes (like EarlyCSE and GVN) use this as an analysis. But you're correct that it's also good to consolidate related folds. I don't think there's a real compile-time concern either way since ctpop should be too rare to make any difference to overall compile-time.
> > 
> > If you want to add the fold for known-pow2-or-zero to instcombine, add those tests to InstCombine rather than here. There shouldn't be a need to duplicate the ones that are already handled here. The regression tests are meant to be minimal, so we don't want to run multiple/different passes here.
> 
> Got it & Thanks!
> 
> Quick question about protocol when a code change is accepted but the test patch isn't. Is it assumed that NFC test changes are okay to just push in that case or should I wait on separate approval there?
D141989 is accepted too. Generally, you can add/adjust tests as an NFC patch without pre-commit approval if you already have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141990