[PATCH] D34009: [Power9] Exploit vector integer extend instructions when indices aren't correct

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

LGTM. My comments are only about, well, comments. So feel free to fix them on the commit.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:11235
+
+  for (unsigned i = 0; i < N->getNumOperands(); i++) {
+    if (DAG.getDataLayout().isLittleEndian())
----------------
Maybe just a quick note about the algorithm to make it easier to understand at first glance. Something like:
```
// Knowing the element indices being extracted from the original
// vector and the order in which they're being inserted, just put
// them at element indices required for the instruction.
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:11255
+// extended vector_extract elements of specific indices. If the correct indices
+// aren't used, add a vector shuffle to fix up the indices and create a new
+// PPCISD:SExtVElems node which selects the new vector sign extend instrustions
----------------
Small nit: get rid of the use of `new` in the comment because these things won't be new for very long :).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:11258
+// during instruction selection.
+// Extending byte to word:
+// LE indices: 0,4,8,12. BE indices: 3,7,11,15
----------------
All of this can just go away. You're repeating it in a clean and concise way at the definition of the `TargetElems` array.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:11268
+// LE indices: 0,2. BE indices: 1,3
+static SDValue combineBVOfVecExtend(SDNode *N, SelectionDAG &DAG) {
+  // This array encodes the indices that the vector sign extend instructions
----------------
It's not just any extend, right?
Probably `combineBVOfVecSExt()` then.


https://reviews.llvm.org/D34009