[PATCH] D133700: [PowerPC] Exploit xxperm, check for dead vectors and substitute vperm with xxperm

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Thank you for your patience and sorry it took me so long to get to this.

I have a bunch of comments but most of them are not a big deal and a couple of them don't require action at all it's just something that's important to notice.
Overall I think that the patch makes sense and hopefully after this round of changes we will be ready to put it in.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10176
   ShufflesHandledWithVPERM++;
   SDValue VPermMask = DAG.getBuildVector(MVT::v16i8, dl, ResultMask);
   LLVM_DEBUG(dbgs() << "Emitting a VPERM for the following shuffle:\n");
----------------
Is `VPermMask` used anymore other than the LLVM_DEBUG?

If it's not this line will cause a warning, or error with -Werror,  when the `LLVM_DEBUG` is removed in some builds.

It looks to me like the whole for loop starting on line 10163 might be redundant as you do the same thing above and in `LowerVPERM`.
You may want to restructure the code so that you have 
```
if (Subtarget.isISA3_0() && (V1->hasOneUse() || V2->hasOneUse())) {
  // Do the codegen with XXPERM here
  LLVM_DEBUG(dbgs() << "Emitting a VPERM for the following shuffle:\n");
  LLVM_DEBUG(SVOp->dump());
  LLVM_DEBUG(dbgs() << "With the following permute control vector:\n");
  LLVM_DEBUG(VPermMask.dump());
} else {
  // Do the codegen with VPERM here
  LLVM_DEBUG(dbgs() << "Emitting a XXPERM for the following shuffle:\n");
  LLVM_DEBUG(SVOp->dump());
  LLVM_DEBUG(dbgs() << "With the following permute control vector:\n");
  LLVM_DEBUG(XXPermMask.dump());
}
```
Or, at least get rid of the above code because it's not really needed. The debug info can come at the end of `LowerVPERM`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10183
+  LLVM_DEBUG(dbgs() << "Emitting a VPERM for the following shuffle:\n");
+  LLVM_DEBUG(SVOp->dump());
+  return LowerVPERM(Op, DAG, PermMask, VT, V1, V2);
----------------
Aren't these lines the same as the lines 10177 - 10178 ? 
If they are you probably don't need them.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10191
+  unsigned Opcode = PPCISD::VPERM;
+  auto ValType = V1.getValueType();
+  SDLoc dl(Op);
----------------
nit:
Use `EVT` instead for `auto`.
Code is generally easier to read when we have the types spelled out.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10280
+  if (V2HasXXSWAPD) {
+    dl = SDLoc(V2->getOperand(0));
+    V2 = V2->getOperand(0)->getOperand(1);
----------------
I don't think you have to do anything about this but more of a note to make sure it is taken into consideration. (Perhaps a comment in the code would be good.)
Not a big deal because this is debug info but in this case we could be overwriting the `dl` from the previous if statement if we have two swaps.  I guess we want to use the location of the original swap which is fine and it doesn't matter which one we use.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10291
+
+  ShufflesHandledWithVPERM++;
+  SDValue VPermMask = DAG.getBuildVector(MVT::v16i8, dl, ResultMask);
----------------
Does this get incremented twice for the same instruction? 
Once here and once on line 10175 above.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10294
+  LLVM_DEBUG(dbgs() << "With the following permute control vector:\n");
+  LLVM_DEBUG(VPermMask.dump());
+
----------------
Oh, I see. The second half of the debug comment is down here.
It may be a good idea to move the two parts of the comment to the same place down here. 

Also, the initial part of the comment :
```
Emitting a VPERM for the following shuffle:
```
May not be true as this may now be an `XXPERM`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10206
+        dbgs()
+        << "At least one of two input vector is dead - using XXPERM instead\n");
+    Opcode = PPCISD::XXPERM;
----------------
amyk wrote:
> Minor nit.
nit:
`vector` -> `vectors` 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:91
+  SDTCisVT<0, v2f64>, SDTCisVT<1, v2f64>,
+  SDTCisVT<2, v2f64>]>;
 //--------------------------- Custom PPC nodes -------------------------------//
----------------
Is it possible to add a type constraint for the last operand here?  
```
SDTCisVT<3, v4i32>
```
Or is that going to cause issues elsewhere?


================
Comment at: llvm/test/CodeGen/PowerPC/vector-constrained-fp-intrinsics.ll:8059
+; PC64LE9-NEXT:    xxperm 0, 2, 1
+; PC64LE9-NEXT:    xxlor 34, 0, 0
 ; PC64LE9-NEXT:    blr
----------------
Interesting. Here we actually end up with an extra copy which is not what we want but it's because the `xxperm` feeds the return value and so the register allocation is constrained by the ABI. For this patch I think we can ignore this but we should make a note of it to fix it at a later date.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133700/new/

https://reviews.llvm.org/D133700