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

[Kit Barton via llvm-commits]

kbarton 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,
----------------
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). 


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7096
+  if (V->isConstant())
     return false;
+  for (int i = 0, e = V->getNumOperands(); i < e; ++i) {
----------------
nemanjai wrote:
> kbarton wrote:
> > The current comment says that this can be done for non constant splats, but this seems to indicate it cannot. 
> This function is only called if the node was already confirmed not to be a constant splat (see below). So if the node is constant, it's building a vector out of different constants and it is beneficial to expand the BUILD_VECTOR so we can just get a LOAD node (from the constant pool).
That's probably worth an inline comment ;)


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7116
+  }
+  return !(IsSplat && IsLoad);
 }
----------------
nemanjai wrote:
> kbarton wrote:
> > This also looks a bit suspicious, but that's mostly because I'm not clear on the exact conditions under which we have efficient patterns. 
> I think hopefully the updated comment will clarify this.
Yes, this is good now. 


Repository:
  rL LLVM

https://reviews.llvm.org/D25912