[PATCH] D33690: [PowerPC] Match vec_revb builtins to P9 instructions.

[Kit Barton via Phabricator via llvm-commits]

kbarton added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1602
 
-// Check that the mask is shuffling N byte elements.
-static bool isNByteElemShuffleMask(ShuffleVectorSDNode *N, unsigned Width) {
+/// \brief Check that the mask is shuffling N byte elements. Within each N byte
+/// element of the mask, the indices could be either in incremental or
----------------
Please remove \brief. We no longer need to use \brief now that autobrief has been enabled. 


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1604
+/// element of the mask, the indices could be either in incremental or
+/// decremental order as long as they are consecutive.
+/// \param[in] N: the shuffle vector SD Node to analyze
----------------
incremental/decremental -> increasing/decreasing


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1609
+/// \param[in] StepLen: the delta indices number among the N byte element, if
+/// the mask is in incremental/decremental order then it is 1/-1.
+/// \return true iff the mask is shuffling N byte elements.
----------------
incremental/decremental -> increasing/decreasing


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1781
+
+  for (int i = 0; i < 16; i += Width) {
+    if (N->getMaskElt(i) != i + Width - 1)
----------------
No braces required here. 


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7909
+      return DAG.getNode(ISD::BITCAST, dl, MVT::v16i8, ReveQWord);
+    }
+  }
----------------
I'm probably missing something basic here, but why are we always converting the return to v16i8?


================
Comment at: test/CodeGen/PowerPC/vec_revb.ll:2
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 < %s | FileCheck %s -check-prefix=CHECK-BE
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 < %s | FileCheck %s  -check-prefix=CHECK-LE
+
----------------
The patterns are the same for both BE and LE, therefor you don't need separate CHECK-BE and CHECK-LE labels. 


https://reviews.llvm.org/D33690