[PATCH] D33225: [PowerPC] Fix a performance bug for PPC::XXSLDWI.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

Although none of these comments are about something major that needs to change, I'd like to see an updated patch with all the comments addressed before approving it.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1577
   // Check that the mask is shuffling words
+static bool isShufflingWordsMask(ShuffleVectorSDNode *N) {
   for (unsigned i = 0; i < 4; ++i) {
----------------
The name sounds weird. Perhaps `isWordShuffleMask()`?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1667
 
+/// Possible LE ShuffleVector masks (Case 1):
+/// ShuffleVector((vector int)a, vector(int)b, 0, 1, 2, 3)
----------------
Although I appreciate the effort, we can't really have comments enumerating all the possible shuffles that a specific instruction can take. As you can imagine, doing so for all the different instruction handling specific shuffle types would blow up into thousands of lines of comments.

Besides, the enumeration does seem to be missing some. For example a shuffle mask of `3,0,1,2` does not appear to be listed and should be valid (i.e. shift a vector concatenated with itself left by one word).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1731
+                               bool &Swap, bool IsLE) {
+  ShiftElts = 0;
+  if (N->getValueType(0) != MVT::v16i8)
----------------
As general guidance, do not modify output parameters unless the function succeeds.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1746
+  // ShuffleVector Mask Index must between 0 - 31
+  assert(M0 <= 7 && "Invalid ShuffleVector Mask Index");
+
----------------
It seems kind of arbitrary to assert this for just one index. It should be all or none.


================
Comment at: test/CodeGen/PowerPC/vec_sldwi.ll:1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 < %s | \
+; RUN:   FileCheck %s  -check-prefix=CHECK-LE
----------------
If you're splitting commands on multiple lines, please keep the lines to 80 columns.


================
Comment at: test/CodeGen/PowerPC/vec_sldwi.ll:9
+; CHECK-LE-LABEL: @check_le_vec_sldwi_va_vb_0
+	%tmp = shufflevector <16 x i8> %VA, <16 x i8> %VB,
+	      		      <16 x i32> <i32 0, i32 1, i32 2, i32 3,
----------------
I don't think you should be splitting the lines of IR in the test cases. You can leave them the way they come out of clang. Also, it's much more natural to look at the test cases if the types of vectors you pass in are `<4 x i32>` since it's more concise and is a nice match for the instruction. I don't really understand the use of `<16 x i8>` with all the clutter it introduces.


https://reviews.llvm.org/D33225