[PATCH] D18441: [X86][XOP] Support for VPPERM shuffle mask decoding

[Sanjay Patel via llvm-commits]

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

LGTM - see inline comments for nits.


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1042
@@ +1041,3 @@
+  SmallVector<int, 8> ShuffleMask(Mask.begin(), Mask.end());
+  if (Src1Name == Src2Name) {
+    for (unsigned i = 0, e = ShuffleMask.size(); i != e; ++i) {
----------------
Add a comment here to explain. Something like: if there's really only one source operand, fix the mask so we can print all elements in one span.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1044-1046
@@ +1043,5 @@
+    for (unsigned i = 0, e = ShuffleMask.size(); i != e; ++i) {
+      if ((int)ShuffleMask[i] >= 0 && // Not sentinel.
+          ShuffleMask[i] >= (int)e)   // From second mask.
+        ShuffleMask[i] -= e;
+    }
----------------
The >=0 check is redundant?

================
Comment at: lib/Target/X86/X86ShuffleDecodeConstantPool.cpp:180
@@ +179,3 @@
+
+  for (unsigned i = 0; i != NumElts; ++i) {
+    Constant *COp = C->getAggregateElement(i);
----------------
warning: comparison of signed and unsigned

================
Comment at: lib/Target/X86/X86ShuffleDecodeConstantPool.cpp:203
@@ +202,3 @@
+    // 7 - Invert most significant bit of source byte and replicate in all bit positions.
+    APInt APElt = cast<ConstantInt>(COp)->getValue();
+    for (int j = 0; j != Scale; ++j) {
----------------
"APElt" made me think I was seeing a new type rather than a variable. "MaskElt"?

================
Comment at: lib/Target/X86/X86ShuffleDecodeConstantPool.h:35
@@ -34,1 +34,3 @@
 
+/// \brief Decode a VPPERM variable mask from an IR-level vector constant.
+void DecodeVPPERMMask(const Constant *C, SmallVectorImpl<int> &ShuffleMask);
----------------
auto brief is on, so don't need to use \brief explicitly.
I know it doesn't match anything else here. Separate cleanup commit?

================
Comment at: lib/Target/X86/X86ShuffleDecodeConstantPool.h:36
@@ +35,3 @@
+/// \brief Decode a VPPERM variable mask from an IR-level vector constant.
+void DecodeVPPERMMask(const Constant *C, SmallVectorImpl<int> &ShuffleMask);
+
----------------
Decode -> decode. 
I know it doesn't match anything else again. Fix all of these function names to be lowercase in a separate commit?

================
Comment at: test/CodeGen/X86/vector-shuffle-combining-xop.ll:24-31
@@ -23,10 +23,10 @@
 
 define <16 x i8> @combine_vpperm_as_unary_unpckhwd(<16 x i8> %a0, <16 x i8> %a1) {
 ; CHECK-LABEL: combine_vpperm_as_unary_unpckhwd:
 ; CHECK:       # BB#0:
-; CHECK-NEXT:    vpperm {{.*}}(%rip), %xmm0, %xmm0, %xmm0
+; CHECK-NEXT:    vpperm {{.*#+}} xmm0 = xmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15]
 ; CHECK-NEXT:    retq
   %res0 = call <16 x i8> @llvm.x86.xop.vpperm(<16 x i8> %a0, <16 x i8> %a0, <16 x i8> <i8 8, i8 24, i8 9, i8 25, i8 10, i8 26, i8 11, i8 27, i8 12, i8 28, i8 13, i8 29, i8 14, i8 30, i8 15, i8 31>)
   ret <16 x i8> %res0
 }
 
----------------
Is there a way to test the scaled (element != i8) mask decoder path?


Repository:
  rL LLVM

http://reviews.llvm.org/D18441