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


================
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:
> 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 ;)
OK, I'll add that.

```
// This function is called in a block that confirms the node is not a constant splat.
// So a constant BUILD_VECTOR here means the vector is built out of different constants.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7267
+    // elements are the same constant.
+    if (HasAnyUndefs || ISD::isBuildVectorAllOnes(BVN)) {
+      SmallVector<SDValue, 16> Ops(16, DAG.getConstant(SplatBits,
----------------
kbarton wrote:
> nemanjai wrote:
> > kbarton wrote:
> > > I don't follow this logic. Doesn't the isBuildVectorAllOnes already account for any Undefs in the vector? 
> > Neither condition is strictly a strengthened version of the other. Think about a build vector node such as this:
> > 
> > ```
> > (v16i8 (build_vector i8:3, i8:3, i8:3, i8:undef, i8:undef, i8:3, ...))
> > ```
> > That one has undefs and the splat size is 1 byte but it is not a BUILD_VECTOR of all ones. Although it is certainly possible for a node to have undefs and be a BUILD_VECTOR of all ones.
> I still don't follow the logic. In the example above, do we want to match it or not? Based on the comments above, we want to (all elements are either 3, or undef). How do we guarantee that all the all elements are the same constant - I don't see that check anywhere.  
> 
So at this point, we have determined that this is a splat of a constant and that we can build this vector by splatting a 1-byte value. We also have the value of that constant (`SplatBits`). However, the actual node may have some undefs. What this code will do is change any inputs into the value that needs to be splat to build this vector. In the example above, we'll just replace all inputs with constant 3 and leave everything the same so the BUILD_VECTOR node will be matched in the .td file.


================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1383
 
-let UseVSXReg = 1 in {
+let UseVSXReg = 1, AddedComplexity = 400 in {
 let Predicates = [HasDirectMove] in {
----------------
kbarton wrote:
> nemanjai wrote:
> > kbarton wrote:
> > > Is there a way we can do this without the use of AddedComplexity?
> > Are you suggesting that we do away with AddedComplexity from VSX instruction definitions altogether? This was just added for consistency as it was initially missing. Ultimately, having it there doesn't change anything (at this time) because these instructions match PPCISD nodes that are not matched by anything else. However, in the highly unlikely situation where some VMX instructions are added in the future that match these nodes, it is good to have the AddedComplexity there as it appears in the remainder of the VSX target definition file.
> > 
> > If you want, I can certainly remove this instance of it without affecting anything this patch does.
> My (slight) preference is to not use it unless it is absolutely necessary.
> Ideally we'll get remove the need for it at some point, but that is a big piece of work. For now, I don't think we should be using it unless it's absolutely necessary. 
Yeah, for sure. I'll remove this. Direct moves do not have a corresponding VMX alternative so this accomplishes nothing.


Repository:
  rL LLVM

https://reviews.llvm.org/D25912