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

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

I also rebased to support the commutative predicates of D59992 <https://reviews.llvm.org/D59992>.



================
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
----------------
RKSimon wrote:
> Maybe OperandMode or OperandType?
I am trying to avoid 'Type' because it might get confused with LLVM Type. How about ReorderingMode?


================
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
----------------
RKSimon wrote:
> Its not necessarily the same opcode (might be altopcode etc.)
Correct, I will update the comment.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:640
+    Constant, // Matching constants
+    Splat,    // Matching the same instruction multiple times (broadcast)
+    Failed,   // We failed to create a vectorizable group
----------------
RKSimon wrote:
> Matching for extractelement is also an issue: https://bugs.llvm.org/show_bug.cgi?id=41304
Good point, but let's implement it in a separate patch.


================
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,
----------------
RKSimon wrote:
> Could this be static?
It could, but we need to pass DL and SE for isConsecutiveAccess().


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:915
+  /// \reorder the operands in \p Ops to improve vectorization.
+  void reorderOperandVecs(VLOperands &Ops);
+
----------------
RKSimon wrote:
> Do we need this? Maybe just put inside reorderInputsAccordingToOpcode?
I agree, it is kind of redundant right now.

I have placed it in a standalone function because there will be a second call site for it in the follow-up patch that implements operand reordering across more than 2 operand vectors.


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

https://reviews.llvm.org/D59973