[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:
> 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). 
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 )


Repository:
  rL LLVM

https://reviews.llvm.org/D25912