[PATCH] D124309: [SLP] Steer for the best chance in tryToVectorize() when rooting with binary ops.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1037
 
-  /// A helper data structure to hold the operands of a vector of instructions.
-  /// This supports a fixed vector length for all operand vectors.
-  class VLOperands {
-    /// For each operand we need (i) the value, and (ii) the opcode that it
-    /// would be attached to if the expression was in a left-linearized form.
-    /// This is required to avoid illegal operand reordering.
-    /// For example:
-    /// \verbatim
-    ///                         0 Op1
-    ///                         |/
-    /// Op1 Op2   Linearized    + Op2
-    ///   \ /     ---------->   |/
-    ///    -                    -
-    ///
-    /// Op1 - Op2            (0 + Op1) - Op2
-    /// \endverbatim
-    ///
-    /// Value Op1 is attached to a '+' operation, and Op2 to a '-'.
-    ///
-    /// Another way to think of this is to track all the operations across the
-    /// path from the operand all the way to the root of the tree and to
-    /// calculate the operation that corresponds to this path. For example, the
-    /// path from Op2 to the root crosses the RHS of the '-', therefore the
-    /// corresponding operation is a '-' (which matches the one in the
-    /// linearized tree, as shown above).
-    ///
-    /// For lack of a better term, we refer to this operation as Accumulated
-    /// Path Operation (APO).
-    struct OperandData {
-      OperandData() = default;
-      OperandData(Value *V, bool APO, bool IsUsed)
-          : V(V), APO(APO), IsUsed(IsUsed) {}
-      /// The operand value.
-      Value *V = nullptr;
-      /// TreeEntries only allow a single opcode, or an alternate sequence of
-      /// them (e.g, +, -). Therefore, we can safely use a boolean value for the
-      /// APO. It is set to 'true' if 'V' is attached to an inverse operation
-      /// in the left-linearized form (e.g., Sub/Div), and 'false' otherwise
-      /// (e.g., Add/Mul)
-      bool APO = false;
-      /// Helper data for the reordering function.
-      bool IsUsed = false;
-    };
-
-    /// During operand reordering, we are trying to select the operand at lane
-    /// that matches best with the operand at the neighboring lane. Our
-    /// selection is based on the type of value we are looking for. For example,
-    /// if the neighboring lane has a load, we need to look for a load that is
-    /// accessing a consecutive address. These strategies are summarized in the
-    /// 'ReorderingMode' enumerator.
-    enum class ReorderingMode {
-      Load,     ///< Matching loads to consecutive memory addresses
-      Opcode,   ///< Matching instructions based on opcode (same or alternate)
-      Constant, ///< Matching constants
-      Splat,    ///< Matching the same instruction multiple times (broadcast)
-      Failed,   ///< We failed to create a vectorizable group
-    };
-
-    using OperandDataVec = SmallVector<OperandData, 2>;
-
-    /// A vector of operand vectors.
-    SmallVector<OperandDataVec, 4> OpsVec;
-
+  /// A helper class used for scoring candidates for two consecutive lanes.
+  class LookAheadHeuristics {
----------------
vporpo wrote:
> nit: It also holds the operands of a VL, so it is probably best to mention this here too.
Hm, I'm not sure I understand what you mean here. You might be mislead by the way diff is shown here.
This is not renaming of existing VLOperands.
This new helper class formed basically by couple routines: getShallowScore and getScoreAtLevelRec which were pulled out of VLOperands along with score constants.  The class does not store anything from VL.
It only needs total number of lanes for scoring.  I even changed both methods to be const.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2007
+  /// above the LookAheadHeuristics::ScoreFail.
+  Optional<int> findBestPair(ArrayRef<std::pair<Value *, Value *>> Candidates) {
+    LookAheadHeuristics LookAhead(*DL, *SE, *this, /*NumLanes=*/2,
----------------
vporpo wrote:
> nit: perhaps `findBestRootPair` ?
Thanks for suggestion. Will apply it with next rebase.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2009
+    LookAheadHeuristics LookAhead(*DL, *SE, *this, /*NumLanes=*/2,
+                                  LookAheadMaxDepth);
+    int BestScore = LookAheadHeuristics::ScoreFail;
----------------
vporpo wrote:
> We could also have a separate max-depth limit for this, because I guess it will not ran as frequently as the other one, so we could have a higher depth if required.
Will do. Any suggestion about option name?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9217
+  Optional<int> BestCandidate = R.findBestPair(Candidates);
+  if (!BestCandidate.hasValue())
+    return false;
----------------
vporpo wrote:
> nit: I think we can drop `.hasValue()`, `if (!BestCandidate)` should work fine.
sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124309