[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:7203
+      return SDValue();
+
+    // SDAG patterns are provided for building vectors out of values that are
----------------
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.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7214-7217
+    auto haveEfficientPattern = [&](BuildVectorSDNode *V) -> bool {
+      bool IsSplat = true;
+      bool IsLoad = false;
+      SDValue Op0 = V->getOperand(0);
----------------
echristo wrote:
> nemanjai wrote:
> > amehsan wrote:
> > > Do we need a lambda that is called only once? Since this function is very long already, I think creating a separate function for this is reasonable to avoid making this function too long. 
> > I only added a lambda for two reasons:
> > 1. The logic is very close to where it's used
> > 2. The logic isn't needed anywhere else
> > 
> > But I really don't feel strongly about that reasoning and would be happy to change it to a function if that's what is preferred. What do the others think?
> No strong opinion here. I'm slightly more likely to want to outline code because it is a very long function, but no strong opinion.
Two reviewers constitute a consensus in my opinion. I'll convert this to a static function.


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


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


================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:2752
+  let Predicates = [HasDirectMove] in {
+    /* Endianness-neutral constant splat on P8 and newer targets. The reason
+       for this pattern is that on targets with direct moves, we don't expand
----------------
kbarton wrote:
> Please convert to C++ comments
OK, will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D25912