[PATCH] D128911: Emit table lookup from TargetLowering::expandCTTZ()

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:4762
+  /// \param N Node to expand
+  /// \returns Reference to table generated in Constant Pool.
+  SDValue CTTZTableLookup(SDNode *N, SelectionDAG &DAG, const SDLoc &DL, EVT VT,
----------------
barannikov88 wrote:
> gsocshubham wrote:
> > barannikov88 wrote:
> > > The comment is still confusing. The function does not return reference to the generated table, it returns the expanded node.
> > > The "brief" part of the should also be fixed. Currently, it documents the way the function is used, which it should not. Uses may be added or removed, while the comment should stay the same.
> > > 
> > How about now?
> > 
> > ```
> >   /// Expand CTTZ node if CTLZ/CTPOP operations are not legal.
> >   /// \param N Node to expand
> >   /// \returns The expansion result or SDValue() if it fails.
> > ```
> > 
> > @barannikov88 - Is brief part fine now? If not, can you please suggest a better one?
> Sounds ok to me now.
> My suggestion would be "Expands CTTZ via table lookup". This would not imply that ctlz/ctpop have to be illegal if you want to use this method. This, however, is what the name of the method already says, making the whole comment kind of redundant. Use it or leave it as it is; I guess it is not a major issue since noone else objected.
Thanks for suggestion. It looks better now!


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8001
+      DAG.getNode(ISD::MUL, DL, VT, DAG.getNode(ISD::AND, DL, VT, Op, Neg),
+                  DAG.getConstant(DeBruijn.getZExtValue(), DL, VT)),
+      DAG.getConstant(ShiftAmt, DL, VT));
----------------
craig.topper wrote:
> You don't need getZExtValue() here
Removed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8018
+                                   DAG.getMemBasePlusOffset(CPIdx, Lookup, DL),
+                                   PtrInfo, EVT(MVT::i8));
+  if (Node->getOpcode() != ISD::CTLZ_ZERO_UNDEF) {
----------------
craig.topper wrote:
> I don't think you need an explicit `EVT` around MVT::i8
Removed. Done.


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

https://reviews.llvm.org/D128911