[PATCH] D25580: [PowerPC] Improve handling of BUILD_VECTOR nodes (integer results)

[Nemanja Ivanovic via llvm-commits]

nemanjai added a comment.

In https://reviews.llvm.org/D25580#576141, @echristo wrote:

> Wow. This patch is huge. I don't know if some/any of it can be split up offhand.
>
> Some inline comments.
>
> -eric


I was struggling to decide how to split this up because I realize the patch is kind of hefty. But all of it is improvements to the handling of BUILD_VECTOR nodes and most parts of the patch are dependent on other chunks. I think that the two new functions (combining a build vector of conversions to a conversion of build vector and combining consecutive loads) can be split into separate patches. However, I'm not sure if that accomplishes much since that code is already sitting in separate functions.



================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:225
   const MCOperand &MO = MI.getOperand(OpNo);
-  if (MO.isImm())
+  // PPC::ZERO and PPC::ZERO8 are register MachineOperand's but they really
+  // represent immediates.
----------------
echristo wrote:
> This seems... weird.
I agree. But the weirdness exists in the ISA as well. So I'm not sure how best to tackle this issue. Perhaps @hfinkel can provide a pointer regarding this.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7240
+          return false;
+        if (V->getOperand(i).getOpcode() == ISD::LOAD ||
+            (V->getOperand(i).getOpcode() == ISD::FP_ROUND &&
----------------
echristo wrote:
> Might want to comment on the logic here.
How about:

```
// We want to expand nodes that represent load-and-splat even if the 
// loaded value is a floating point truncation or conversion to int.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7277-7278
 
+  fprintf(stderr, "SplatBits = %u, SplatUndef = %u, SplatSize = %u\n",
+          SplatBits, SplatUndef, SplatSize);
   // We have XXSPLTIB for constant splats one byte wide
----------------
echristo wrote:
> Debugging code - here and below.
Doh! Sorry about this, I'm usually pretty careful to clean these up.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7282
+    fprintf(stderr, "Have P9 vector and SplatSize is 1\n");
+    // This is a splat other than some of the inputs are undef. Convert to
+    // a constant splat.
----------------
echristo wrote:
> Not sure I understand this comment.
How about:

```
// This is a splat of 1-byte elements with some elements potentially undef.
// Rather than trying to match undef in the SDAG patterns, ensure that all elements
// are the same constant.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10549
+SDValue
+PPCTargetLowering::combineBVOfFpToIntToFpToIntOfBV(SDNode *N,
+                                                   DAGCombinerInfo &DCI) const {
----------------
echristo wrote:
> ... this function name.
Yeah, the name tries to say too much. I used:
`DAGCombiner::reduceBuildVecConvertToConvertBuildVec`
as inspiration for the name but specified what kind of conversion it is. Please suggest a different name.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10619
+
+static SDValue combineBVOfConsecutiveLoads(SDNode *N, SelectionDAG &DAG) {
+  assert(N->getOpcode() == ISD::BUILD_VECTOR &&
----------------
echristo wrote:
> What's going on here?
This is a secret and mysterious function :).
Just kidding, how about:

```
/// combineBVOfConsecutiveLoads - if this node builds a vector out of consecutive loads,
/// convert it to a load of the vector type. Also, if the loads are consecutive but in descending
/// order, load the vector type and reverse the elements with a shuffle.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10734
   // Looking for:
   // (build_vector ([su]int_to_fp (extractelt 0)), [su]int_to_fp (extractelt 1))
+  if (FirstInput.getOpcode() != ISD::SINT_TO_FP &&
----------------
echristo wrote:
> More comments like this.
OK, I'll look for other places where such a comment can compactly describe what we're after and add them.


Repository:
  rL LLVM

https://reviews.llvm.org/D25580