[PATCH] [X86][SSE] Improved (v)insertps shuffle matching

[Quentin Colombet]

Hi Simon,

LGTM with more comments to ease futur modifications.
See the inlined comments.

Any performances numbers related to this?

Thanks,
-Quentin


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8178
@@ +8177,3 @@
+  int V2DstIndex = -1;
+  int V1InPlace = 0;
+
----------------
We can use a bool for that and I would also change the name to something like IsV1Used.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8205
@@ +8204,3 @@
+  if (V1DstIndex != -1) {
+    V2SrcIndex = Mask[V1DstIndex];
+    V2DstIndex = V1DstIndex;
----------------
Add a comment saying that if we get here, V2 is not used but one element of V1 is moved.
Therefore, V1 will be used as inplace element (first operand) and as the source of insertion (second operand).

I think that would make clearer why V2 gets V1’s information and ease future reading/modifications.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8209
@@ +8208,3 @@
+  } else {
+    V2SrcIndex = Mask[V2DstIndex] - 4;
+  }
----------------
Add a comment saying that unlike LLVM, the index is from the start of the current operand, not the start of the concatenated vector. I.e., indexes in {V1[4] V2[4]} targeting V2 are equals to original index - numberOfElts(V1).

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8212
@@ +8211,3 @@
+
+  if (!V1InPlace) {
+    V1 = DAG.getUNDEF(MVT::v4f32);
----------------
Add a comment saying that V1 is not used, which means it is zeroable.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8214
@@ +8213,3 @@
+    V1 = DAG.getUNDEF(MVT::v4f32);
+  }
+
----------------
No curly brackets.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:8536
@@ +8535,3 @@
+  // Use INSERTPS if we can complete the shuffle efficiently.
+  if (Subtarget->hasSSE41())
+    if (SDValue V = lowerVectorShuffleAsInsertPS(Op, V1, V2, Mask, DAG))
----------------
We can remove this block with the previous one to get rid of one if.

http://reviews.llvm.org/D6879

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