[PATCH] [SLPVectorizer] Reorder operands of shufflevector if it can result in a vectorized code.

[Michael Zolotukhin]

Hi Karthik,

Thanks for the updated patch, it looks good to me except some minor points (you could find my remarks inline).

Thanks,
Michael


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:344-349
@@ -441,2 +343,8 @@
 
+  void reorderAltShuffleOperands(ArrayRef<Value *> VL,
+                                 SmallVectorImpl<Value *> &Left,
+                                 SmallVectorImpl<Value *> &Right);
+  void reorderInputsAccordingToOpcode(ArrayRef<Value *> VL,
+                                      SmallVectorImpl<Value *> &Left,
+                                      SmallVectorImpl<Value *> &Right);
   /// \brief Perform LICM and CSE on the newly generated gather sequences.
----------------
Do we really need these functions to be public?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1875-1876
@@ +1874,4 @@
+  // Reorder operands of commutative operations if the resulting vectors are
+  // consecutive loads
+  // and are not already part of preserving broadcast obtained from above.
+  // If we have something like-
----------------
Something wrong with formatting here.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1888
@@ +1887,3 @@
+        // Maintain order while reordering. Always reorder the later operation
+        // in the tree this prevents us from swapping already swapped elements
+        if (isConsecutiveAccess(L, L1) &&
----------------
A missing sentence end after 'tree'?

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1899-1907
@@ +1898,11 @@
+
+  bool LeftBroadcast = isSplat(Left);
+  bool RightBroadcast = isSplat(Right);
+  // Don't reorder if the operands where good to begin with.
+  if (!(LeftBroadcast || RightBroadcast) &&
+      (AllSameOpcodeRight || AllSameOpcodeLeft) && !needsReordering) {
+    Left = OrigLeft;
+    Right = OrigRight;
+  }
+}
+
----------------
To minimize change in the current behavior, we could place this check before the last loop. In this case we won't try to reorder the operands if one part is splat or 'allSameOpcode'. Also, we'll get rid of needsReordering flag.

================
Comment at: test/Transforms/SLPVectorizer/X86/operandorder.ll:254-256
@@ +253,5 @@
+
+; CHECK-LABEL: load_reorder_float
+; CHECK: load <4 x float>*
+; CHECK: fadd <4 x float>
+define void @load_reorder_float(float* nocapture %c, float* noalias nocapture readonly %a, float* noalias nocapture readonly %b){
----------------
C-equvalent in comments would be useful here and before other test cases.

http://reviews.llvm.org/D6677

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