[PATCH] [X86][SSE] psrl(w/d/q) and psll(w/d/q) bit shifts for SSE2

[Simon Pilgrim]

Thanks Chandler - I'm creating an updated patch now.


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7755-7770
@@ +7754,18 @@
+                                            SelectionDAG &DAG) {
+  const MVT::SimpleValueType ShiftMapping[][2] = {
+    // SSE2
+    { MVT::v4i32, MVT::v2i64 },
+    { MVT::v8i16, MVT::v4i32 },
+    { MVT::v8i16, MVT::v2i64 },
+    { MVT::v16i8, MVT::v8i16 },
+    { MVT::v16i8, MVT::v4i32 },
+    { MVT::v16i8, MVT::v2i64 },
+    // AVX2
+    { MVT::v8i32, MVT::v4i64 },
+    { MVT::v16i16, MVT::v8i32 },
+    { MVT::v16i16, MVT::v4i64 },
+    { MVT::v32i8, MVT::v16i16 },
+    { MVT::v32i8, MVT::v8i32 },
+    { MVT::v32i8, MVT::v4i64 }
+  };
+
----------------
chandlerc wrote:
> Is this clang-formatted? I would have expected a slightly different layout, but  I  don't want to quibble with  whatever clang-format chooses.
I tried that but I thought clang-format's indentation was rather extreme - it pushes all the entries to the far right (all that whitespace!). But to avoid confusion I'll use it again. 

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7772-7783
@@ +7771,14 @@
+
+  auto isSequential = [](int Base, int StartIndex, int EndIndex, int IndexOffset,
+    int MaskOffset, ArrayRef<int> Mask) {
+    StartIndex += IndexOffset;
+    EndIndex += IndexOffset;
+    for (int i = StartIndex; i < EndIndex; i++) {
+      if (Mask[i] < 0)
+        continue;
+      if (i + Base != Mask[i] - MaskOffset)
+        return false;
+    }
+    return true;
+  };
+
----------------
chandlerc wrote:
> Have you checkde whether we already have such a  predicate? I thought we did, but maybe this is sligthly different. Either  way, a comment clarifying the exact thing it is  checking would be really helpful I think. At the very least, the argument set is quite surprising.
> 
> Also, for lambdas, I've been consistently naming them as variables, and so IsSequential.
Turns out there is a usable predicate (isSequentialOrUndefInRange) and whatever reason I didn't use it originally in lowerVectorShuffleAsByteShift was false - that patch went through a lot of edits. I've changed it for both lowerVectorShuffleAsByteShift and lowerVectorShuffleAsBitShift.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7787
@@ +7786,3 @@
+
+  for (unsigned m = 0, e = array_lengthof(ShiftMapping); m != e; m++) {
+    if (VT.SimpleTy == ShiftMapping[m][0]) {
----------------
chandlerc wrote:
> Does a range based for loop not work here?
Not sure what you mean here - have you got any examples?

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7807-7808
@@ +7806,4 @@
+        if (ZeroableRight) {
+          bool ValidShiftRight1 = true;
+          bool ValidShiftRight2 = true;
+
----------------
chandlerc wrote:
> Rather than use variables, maybe sink this into the predicate function so that you can early-exit from the loops?
This doesn't really work now that I can remove the predicated and use isSequentialOrUndefInRange directly.

http://reviews.llvm.org/D6649

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/