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

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10191
+                                      SDValue V1, SDValue V2) const {
+  // SDValue VPERMNode;
+  unsigned Opcode = PPCISD::VPERM;
----------------
I assume this is meant to be deleted?


================
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;
----------------
Minor nit.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10209
+
+    // if V2 is dead, then we swap vV1 and V2 so we can
+    // use V2 as the destination instead.
----------------
Can we elaborate why we want V2 as the destination?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10217
+
+  bool V1HasXSWAP = V1->getOperand(0)->getOpcode() == PPCISD::XXSWAPD;
+  bool V2HasXSWAP = V2->getOperand(0)->getOpcode() == PPCISD::XXSWAPD;
----------------
nit: XSWAP -> XXSWAPD maybe to match the actual opcode? 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10235
+     0-3     4-7     8-11   12-15         0-3     4-7     8-11   12-15
+  IE, index of A, B += 8, and index of C, D -= 8.
+  XXSWAPD on V2:
----------------
nit: `IE`->`i.e.`
Can we also put a space after each `i.e.` to view the illustration better?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10287
+  if (V1HasXSWAP || V2HasXSWAP || Opcode == PPCISD::XXPERM) {
+    if (Subtarget.isPPC64() && ValType != MVT::v2f64)
+      V1 = DAG.getBitcast(MVT::v2f64, V1);
----------------
Might be good to pull `isPPC64()` into a separate variable like how you did for `isLittleEndian`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:119
 
+def SDT_PPCxxperm : SDTypeProfile<1, 3,
+                                  [SDTCisVT<0, v2f64>, SDTCisVT<1, v2f64>,
----------------
nit: Put `SDT_PPCxxperm` near the beginning of the file nearby the other `SDT*` definitions. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133700