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



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7096
+  if (V->isConstant())
     return false;
+  for (int i = 0, e = V->getNumOperands(); i < e; ++i) {
----------------
The current comment says that this can be done for non constant splats, but this seems to indicate it cannot. 


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


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7203
+      return SDValue();
+
+    // SDAG patterns are provided for building vectors out of values that are
----------------
nemanjai wrote:
> kbarton wrote:
> > Would it be better to add this check in the condition above? 
> > Then at least the remaining logic might be able to lower this, instead of just punting to the default case. If not, then I think a comment here is justified. 
> Do you mean add this condition to the outer if? I don't think that would have the correct semantics.
> We don't want to bail on constant splats regardless of whether we have VSX or not. And if this is not a constant splat, we may be able to do something better depending on a number of conditions - however, without VSX, we can't do anything very productive so we bail. How about the following comment:
> 
> ```
> // BUILD_VECTOR nodes that are not constant splats of up to 32-bits can be 
> // lowered to VSX instructions under certain conditions.
> // Without VSX however, there is no pattern more efficient than expanding the node.
> ```
Yup, this is good. Thanks. 


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



================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1383
 
-let UseVSXReg = 1 in {
+let UseVSXReg = 1, AddedComplexity = 400 in {
 let Predicates = [HasDirectMove] in {
----------------
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D25912