[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:643
+  /// These strategies are summarized in the 'ReorderingMode' enumerator.
+  enum class ReorderingMode {
+    Load,     // Matching loads to consecutive memory addresses
----------------
Seems to me, this enum is used only in VLOperands class. Better to move it to class itself.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:733-734
+
+    const DataLayout *DL = nullptr;
+    ScalarEvolution *SE = nullptr;
+  public:
----------------
Better to use references rather than pointers


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:736
+  public:
+    VLOperands() = default;
+    /// Initialize with all the operands of the instruction vector \p VL.
----------------
Seems to me, this constructor is not used.  Transform it into `deleted` rather than `default`.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:776
+    /// Swap the operand at \p OpIdx1 with that one at \p OpIdx2.
+    void swap(unsigned OpIdx1, unsigned OpIdx2, unsigned Lane) {
+      std::swap(OpsVec[OpIdx1][Lane], OpsVec[OpIdx2][Lane]);
----------------
Make it private, since it is used only in class members


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:862
+          break;
+        case ReorderingMode::Splat: {
+          if (Op == OpLastLane)
----------------
I don't think you need braces here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:975
+    /// \returns the number of operands.
+    unsigned getNumOperands() const { return OpsVec.size(); }
+
----------------
Private?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:978
+    /// \returns the number of lanes.
+    unsigned getNumLanes() const { return OpsVec[0].size(); }
+
----------------
Private?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:981
+    /// \returns the operand data at \p OpIdx and \p Lane.
+    OperandData &getData(unsigned OpIdx, unsigned Lane) {
+      return OpsVec[OpIdx][Lane];
----------------
Private?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:986
+    /// \returns the operand data at \p OpIdx and \p Lane. Const version.
+    const OperandData &getData(unsigned OpIdx, unsigned Lane) const {
+      return OpsVec[OpIdx][Lane];
----------------
Check these memeber functions and make private as much as possible


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1001
+    ValueList getVL(unsigned OpIdx) const {
+      ValueList OpVL;
+      for (const OperandData &OpData : OpsVec[OpIdx])
----------------
Preallocate memory for this array since you know the size (`OpsVec[OpIdx].size()`)


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

https://reviews.llvm.org/D59973