[PATCH] [X86][SSE] Avoid shuffles with zero by using pshufb to create zeros

[Quentin Colombet]

Hi Simon,

Look mostly good to me, I just have one concern with the current structure that I found error prone in case we need to update it.
See my inlined comments.

Thanks,
-Quentin


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:9602
@@ +9601,3 @@
+        int V1Idx = ((Mask[i] < 16) && !Zeroable[i] ? Mask[i] : 0x80);
+        int V2Idx = ((Mask[i] < 16) ||  Zeroable[i] ? 0x80 : Mask[i] - 16);
+        V1Mask[i] = DAG.getConstant(V1Idx, MVT::i8);
----------------
This is just a suggestion.
How about moving the zeroable test outside of the ‘?:’ operator.
I.e., first 
        int V1Idx = ((Mask[i] < 16) ? Mask[i] : 0x80);
        int V2Idx = ((Mask[i] < 16) ? 0x80 : Mask[i] - 16);
       if (Zeorable[i])
         V1Idx = V2Idx = 0x80;

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:9605
@@ +9604,3 @@
+        V2Mask[i] = DAG.getConstant(V2Idx, MVT::i8);
+        V1InUse |= (0x80 != V1Idx);
+        V2InUse |= (0x80 != V2Idx);
----------------
I would introduce a constant for 0x80 instead of having it spread.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:9606
@@ -9600,1 +9605,3 @@
+        V1InUse |= (0x80 != V1Idx);
+        V2InUse |= (0x80 != V2Idx);
       }
----------------
We already know this from the ‘?:’ statements.
Seems like worth restructuring the code to actually use a if / else.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:9611
@@ -9605,5 +9610,3 @@
 
-    // Otherwise, blend the two.
-    V2 = DAG.getNode(X86ISD::PSHUFB, DL, MVT::v16i8, V2,
-                     DAG.getNode(ISD::BUILD_VECTOR, DL, MVT::v16i8, V2Mask));
-    return DAG.getNode(ISD::OR, DL, MVT::v16i8, V1, V2);
+    if (V1InUse) {
+      V1 = DAG.getNode(X86ISD::PSHUFB, DL, MVT::v16i8, V1,
----------------
I would structure this and the following if a bit differently.
But that is a matter of taste.

Currently we have:
if (A) {
  // do1
  if (!B)
    return A
}

if (B) {
  // do2
  if (!A)
    return B
  return //do3
}

I would do =>
if (A)
  // do1

if (B)
  // do2

if (A && B)
  return // do3

return A ? A : B;

http://reviews.llvm.org/D6878

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