[PATCH] D120492: [SLP]Improve bottom-to-top reordering.

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3404
   // use.
   auto &&CheckOperands =
+      [this, &NonVectorized](auto &Data,
----------------
I think that this needs a better name, perhaps something like `CanReorderOperands`.
Also this lambda is getting a bit too long, wdyt about replacing it with a member function?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3405
   auto &&CheckOperands =
-      [this, &NonVectorized](const auto &Data,
+      [this, &NonVectorized](auto &Data,
                              SmallVectorImpl<TreeEntry *> &GatherOps) {
----------------
Please rename `Data` to something more descriptive. I think it contains a pair of the TreeEntry and a vector of its uses along with their indexes, so something like `TEAndUses` would be fine.

Also it is probably better to use the actual type here instead of `auto`, ti would make it easier to read.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3414-3420
           ArrayRef<Value *> VL = Data.first->getOperand(I);
-          const TreeEntry *TE = nullptr;
+          TreeEntry *TE = nullptr;
           const auto *It = find_if(VL, [this, &TE](Value *V) {
             TE = getTreeEntry(V);
             return TE;
           });
+          if (It != VL.end() && TE->isSame(VL)) {
----------------
If I understand correctly, this block of code could be a separate member function of the TreeEntry struct that returns the operand TreeEntry: `TreeEntry *getOperandTE(unsigned OpIdx)`. Wdyt ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3423
+            if (any_of(TE->UserTreeIndices, [&Data](const EdgeInfo &EI) {
+                  return EI.UserTE != Data.first;
+                }))
----------------
It is hard to remember what `Data` and `Data.first` refers to at this point. I think it would be better to use a variable to hold the value like `TreeEntry *TE = Data.first`. I think Data.first also appears earlier in the code.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3428
+            // order.
+            Data.second.emplace_back(I, TE);
+            continue;
----------------
I am a bit confused with what is going on here. I was under the impression that the `Users` map holds the edges towards the operands (which btw should probably be part of TreeEntry and built during `buildTree`?). Why are we adding edges here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3535
+        }
+        auto Res = OrdersUses.insert(std::make_pair(OrdersType(), 0));
+        const auto &&AllowsReordering = [IgnoreReorder, &GathersToOrders](
----------------
I am not sure what `Res` refers to. Please use a better name and/or add a comment ?
Also, since it is used after the lambda, it should probably be moved closer to where it is being used.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3536
+        auto Res = OrdersUses.insert(std::make_pair(OrdersType(), 0));
+        const auto &&AllowsReordering = [IgnoreReorder, &GathersToOrders](
+                                            const TreeEntry *TE) {
----------------
Perhaps move the comment from line 3554 here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3550
+        };
+        for (const EdgeInfo &EI : OpTE->UserTreeIndices) {
+          TreeEntry *UserTE = EI.UserTE;
----------------
Could you add a brief comment explaining what this loop does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120492