[PATCH] D132237: [AArch64] expand is-power-of-2 pattern that uses popcount

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18593-18594
 
+  // (ctpop x) == 1 --> (x != 0) && ((x & x-1) == 0)
+  // (ctpop x) != 1 --> (x == 0) || ((x & x-1) != 0)
+  if (LHS.getOpcode() == ISD::CTPOP && LHS.hasOneUse() && isOneConstant(RHS) &&
----------------
davezarzycki wrote:
> spatel wrote:
> > davezarzycki wrote:
> > > For your consideration:
> > > 
> > > (ctpop x) <= 1 --> (x & x-1) == 0
> > > (ctpop x) > 1 --> (x & x-1) != 0
> > Ah, I forgot that we added a popcount target hook with D89952. Do you think we should use that for this pattern too? Or did you decide it wasn't worth pursuing (D91093)?
> > 
> > I'm looking back at the comments in:
> > https://bugs.llvm.org/show_bug.cgi?id=47825
> > ...and we cited AArch64 scalar custom lowering/combining as a missing piece. But we could adapt the TLI hook if that seems better.
> I think the D91093 work is good, but it stopped being a good use of my time to pursue. (Sorry)
> 
> Nevertheless the target hook is there. This seems like a reasonable use of it, no?
Reviewing the earlier versions of D89952 - it doesn't seem like this pattern really fits the intent of the TLI hook?

This pattern will always expand to exactly one subtract+mask (+null check), so it's always going to be cheap based on the cost metric.

But we also have a legality check preceding both of these blocks. We can clean this up a bit and still have a smaller patch that gets us the expected results, so I'll post that alternative and see if it looks better.


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

https://reviews.llvm.org/D132237