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

[Nemanja Ivanovic via llvm-commits]

nemanjai 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:
> 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.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7096
+  if (V->isConstant())
     return false;
+  for (int i = 0, e = V->getNumOperands(); i < e; ++i) {
----------------
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).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7116
+  }
+  return !(IsSplat && IsLoad);
 }
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D25912