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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 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,
----------------
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.


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

https://reviews.llvm.org/D128911