[PATCH] D25912: [PowerPC] Improvements for BUILD_VECTOR Vol. 1

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7083
+/// an efficient pattern if it's not a load-and-splat and is either
+/// a floating point vector or an integer vector on a target with direct moves.
+static bool haveEfficientBuildVectorPattern(BuildVectorSDNode *V,
----------------
kbarton wrote:
> mehdi_amini wrote:
> > nemanjai wrote:
> > > kbarton wrote:
> > > > nemanjai wrote:
> > > > > kbarton wrote:
> > > > > > I don't think I've ever seen \param used like this.
> > > > > > Typically you have a more detailed description following the \brief description, followed by a list of the parameters and return. Something like:
> > > > > > 
> > > > > > \param V The BuildVectorSDNode to analyze
> > > > > > \param HasDirectMove - indicates whether the DirectMove instruction is available
> > > > > > \returns true if there is a pattern in a .td file for this node, false otherwise. 
> > > > > > 
> > > > > > Also, as a minor nit, I find the description of the logic here a bit convoluted. I generally prefer more high-level english descriptions for the logic:
> > > > > > There are several efficient patterns for BUILD_VECTORS. If it is a constant splat it is handled <blah>.
> > > > > > If it is a non-constant splat, the following conditions must be true:
> > > > > >   1. It is not a load-and-splat
> > > > > >   2. It is either a floating point vector or an integer vector on a target with direct moves.
> > > > > > 
> > > > > > This needs to be refined somewhat, because I don't think it actually matches the logic in the function, but hopefully you get the idea. 
> > > > > > 
> > > > > To be perfectly honest with you, I'm not very familiar with Doxygen and this was based on a very quick reading about how these are to be written in LLVM code (so is probably wrong). I'll just follow the pattern you suggested.
> > > > > 
> > > > > How about this for the description:
> > > > > 
> > > > > ```// There are some patterns where it is beneficial to keep a BUILD_VECTOR
> > > > > // node as a BUILD_VECTOR node rather than expanding it. The patterns where
> > > > > // the opposite is true (expansion is beneficial) are:
> > > > > // The node builds a vector out of integers that are not 32 or 64-bits
> > > > > // The node builds a vector out of constants
> > > > > // The node is a "load-and-splat"
> > > > > // In all other cases, we will choose to keep the BUILD_VECTOR.
> > > > > ```
> > > > This looks fine. The only minor comment would be to indent and bullet/number the 3 lines that make up the list. That makes them standout visually. 
> > > > 
> > > > You also need to use /// (3 slashes) to indicate it's something doxygen can format. 
> > > > And make sure to keep the \brief at the beginning - that is important (and quite useful). 
> > > OK, thanks. I'll format it accordingly.
> > Side note: the use of `\brief` is deprecated in llvm, we turned on autobrief some time ago.
> > 
> > (Ref: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments )
> @mehdi_amini Thanks for the pointer - I didn't realize this!
> 
> Although, it's not immediately obvious (to me at least) from that text that \brief has been deprecated in favour of autobrief. How does one go about updating that document?
Send a patch for llvm/docs/CodingStandards.rst



Repository:
  rL LLVM

https://reviews.llvm.org/D25912