[PATCH] D59973: [SLP] Refactoring of the operand reordering code.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:688
+    enum class ReorderingMode {
+      Load,     // Matching loads to consecutive memory addresses
+      Opcode,   // Matching instructions based on opcode (same or alternate)
----------------
Use `///` style of comments here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:848
+        if (getData(OpIdx, Lane).APO)
+          CntTrue++;
+      unsigned CntFalse = NumOperands - CntTrue;
----------------
Use preincrement


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:914
+      ValueList OpVL;
+      OpVL.reserve(OpsVec[OpIdx].size());
+      for (const OperandData &OpData : OpsVec[OpIdx])
----------------
Better to reserve the space when constructing the variable


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:916
+      for (const OperandData &OpData : OpsVec[OpIdx])
+        OpVL.push_back(OpData.V);
+      return OpVL;
----------------
You should not push here, instead assign to the preallocated elements.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:998
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+    LLVM_DUMP_METHOD static const char *getModeStr(ReorderingMode RMode) {
+      switch (RMode) {
----------------
return StringRef.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1031
+      unsigned Cnt = 0;
+      for (const auto &OpDataVec : OpsVec) {
+        OS << "Operand " << Cnt++ << "\n";
----------------
Do not use `auto`s here and in the inner loop since it is hard to deduce the real type


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1099
+  /// Reorder the operands in \p Ops to improve vectorization.
+  void reorderOperandVecs(VLOperands &Ops) const;
+
----------------
Hmm, can't find the definition of this function.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3222
+// Perform operand reordering on the instructions in VL.
+void BoUpSLP::reorderInputsAccordingToOpcode(ArrayRef<Value *> VL,
+                                             SmallVectorImpl<Value *> &Left,
----------------
Could you make it static? It is strange to see const `reorder...` function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59973/new/

https://reviews.llvm.org/D59973