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

[Kit Barton via llvm-commits]

kbarton accepted this revision.
kbarton added a comment.
This revision is now accepted and ready to land.

LGTM



================
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,
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D25912