[PATCH] D105020: [SLP]Improve graph reordering.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minors, but I haven't spotted anything critical - does anyone else have any comments?



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3068
+    return;
+  SmallVector<int> AvailableIndices(MaskedIndices.size());
+  unsigned Cnt = 0;
----------------
Default value?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4259
+        for (unsigned I = VF, End = VL.size(); I < End; I += VF)
+          GatherCost += TTI->getShuffleCost(TTI::SK_Select, VecTy);
+        return ReuseShuffleCost + GatherCost - ScalarsCost;
----------------
Can we remove the loop and avoid the repeated call to TTI->getShuffleCost(TTI::SK_Select, VecTy) ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4265
   }
+  InstructionCost CommonCost = 0;
+  SmallVector<int> Mask;
----------------
Is it worth doing the CommonCost -> ReuseShuffleCost refactor as a NFC pre-commit to simplify this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6106
+      SmallVector<int> Mask(Sz);
+      for (unsigned I = 0; I < Sz; ++I) {
+        unsigned Idx = I;
----------------
A lot of this is just a refactor cleanup that looks like a NFC that could be done as a pre-commit to simplify the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105020