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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Awesome! Some initial thoughts - you maybe want to checkout D57059 <https://reviews.llvm.org/D57059> as @ABataev is looking at adding UNDEF support to handle non-power2 types which will probably need to work with VLOperands



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:616
     unsigned EdgeIdx = UINT_MAX;
-#ifndef NDEBUG
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
     friend inline raw_ostream &operator<<(raw_ostream &OS,
----------------
Separate pre-commit?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:636
+  /// These strategies are summarized in the 'OpMode' enumerator.
+  enum class OpMode {
+    Load,     // Matching loads to consecutive memory addresses
----------------
Maybe OperandMode or OperandType?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:638
+    Load,     // Matching loads to consecutive memory addresses
+    Opcode,   // Matching instructions of the same opcode
+    Constant, // Matching constants
----------------
Its not necessarily the same opcode (might be altopcode etc.)


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:906
+  /// Helper index value for getBestOperand().
+  static const unsigned FailedIdx = UINT_MAX;
+
----------------
Better to use Optional<unsigned> for failure?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:910
+  /// best with the one in Ops[OpIdx][LastLane] and return its index.
+  unsigned getBestOperand(unsigned OpIdx, int Lane, int LastLane,
+                          VLOperands &Ops,
----------------
Could this be static?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:912
+                          VLOperands &Ops,
+                          const SmallVectorImpl<OpMode> &Modes);
+
----------------
ArrayRef<OpMode>


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:915
+  /// \reorder the operands in \p Ops to improve vectorization.
+  void reorderOperandVecs(VLOperands &Ops);
+
----------------
Do we need this? Maybe just put inside reorderInputsAccordingToOpcode?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59973