[PATCH] D113291: [WIP] Add LowerTableBasedCTZ and anable it for AARCH64 in -O3

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D113291#3111997 <https://reviews.llvm.org/D113291#3111997>, @xbolva00 wrote:

> 1. Can you please provide compile time data with @nikic's compile time tracker? This should be *very cheap* to be acceptable.
> 2. Why own pass? We could have this functionality in AggressiveInstCombine.
>
> http://llvm-compile-time-tracker.com/

@xbolva00 Thanks for the comments. I think this should be more appropriate for the AggressiveInstCombine, but I thought it will be easier for the review as is. I'll move it into the Transforms/, so then we can see the compile-time numbers.

@craig.topper Thanks for your comments as well!



================
Comment at: llvm/lib/Transforms/Scalar/LowerTableBasedCTZ.cpp:234
+                 m_ZExtOrSelf(m_Mul(
+                     m_And(m_Sub(m_SpecificInt(0), m_Value(X1)), m_Value(X2)),
+                     m_ConstantInt(MulConst))),
----------------
craig.topper wrote:
> Use m_Neg instead of m_Sub?
> 
> And is Commutable so maybe m_c_And?
Thanks!


================
Comment at: llvm/lib/Transforms/Scalar/LowerTableBasedCTZ.cpp:248
+
+  Argument *ArgX = dyn_cast_or_null<Argument>(X1);
+  if (!ArgX)
----------------
craig.topper wrote:
> Why does it need to be an Argument?
Actually, it doesn't.


================
Comment at: llvm/lib/Transforms/Scalar/LowerTableBasedCTZ.cpp:264
+
+  bool DefinedForZero = ConstData->getElementAsInteger(0) == InputBits;
+
----------------
craig.topper wrote:
> If the value in element 0 isn't InputBits, don't you still need to make the lowering produce the same value as the table would have? For the example, in the bug report the value is 0 and the suggested lowering was ctlz(x) & 0x1f.
Oh yes... our implementation isn't right for that case... I'll update that with some additional instructions (e.g. cmp + select).


================
Comment at: llvm/lib/Transforms/Scalar/LowerTableBasedCTZ.cpp:267
+  IRBuilder<> B(LI);
+  ConstantInt *BoolConst = DefinedForZero ? B.getTrue() : B.getFalse();
+  auto Cttz = B.CreateIntrinsic(Intrinsic::cttz, {ArgXType}, {ArgX, BoolConst});
----------------
craig.topper wrote:
> The polarity of this seems backwards. The second argument to ctlz intrinsic is called "zero_undef". It should be true if you don't care what value is produced for 0.
Right, I'll update this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113291