[PATCH] D64700: [SLPVectorizer] [NFC] Avoid repetitive calls to getSameOpcode().

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1393
+    /// The main/alternate instruction.
+    Instruction *MainOp = nullptr;
+    Instruction *AltOp = nullptr;
----------------
Please add set/get methods and make these member variables private.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1395
+    Instruction *AltOp = nullptr;
+
     /// Do we need to gather this sequence ?
----------------
Also please improve TreeEntry's dump() so that it prints whether this is an alt sequence entry. You could also perhaps print these values if needed.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1473
+    /// OpValue.
+    Value *isOneOf(Value *Op) {
+      auto *I = dyn_cast<Instruction>(Op);
----------------
const?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1520
   TreeEntry *newTreeEntry(ArrayRef<Value *> VL, bool Vectorized,
-                          const EdgeInfo &UserTreeIdx,
+                          InstructionsState *S, const EdgeInfo &UserTreeIdx,
                           ArrayRef<unsigned> ReuseShuffleIndices = None,
----------------
I was under the impression that we are completely removing InstructionsState and replacing it with state from TreeEntry.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1564
   TreeEntry *getTreeEntry(Value *V) {
-    auto I = ScalarToTreeEntry.find(V);
-    if (I != ScalarToTreeEntry.end())
-      return VectorizableTree[I->second].get();
+    auto *I = dyn_cast<Instruction>(V);
+    auto It = ScalarToTreeEntry.find(I);
----------------
Hmm isn't this an unrelated change to this patch?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1572
   const TreeEntry *getTreeEntry(Value *V) const {
-    auto I = ScalarToTreeEntry.find(V);
-    if (I != ScalarToTreeEntry.end())
-      return VectorizableTree[I->second].get();
+    auto *I = dyn_cast<Instruction>(V);
+    auto It = ScalarToTreeEntry.find(I);
----------------
Same here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1580
   /// Maps a specific scalar to its tree entry.
-  SmallDenseMap<Value*, int> ScalarToTreeEntry;
+  SmallDenseMap<Instruction *, TreeEntry *> ScalarToTreeEntry;
 
----------------
Same


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2384
 
   unsigned ShuffleOrOp = S.isAltShuffle() ?
                 (unsigned) Instruction::ShuffleVector : S.getOpcode();
----------------
Shouldn't we be getting this from the TreeEntry? Something like E->isAltShuffle()


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2976
           // vectorized tree.
-          if (areAllUsersVectorized(cast<Instruction>(V)) &&
-              !ScalarToTreeEntry.count(V)) {
+          auto *I = cast<Instruction>(V);
+          if (areAllUsersVectorized(I) && !ScalarToTreeEntry.count(I)) {
----------------
This also looks unrelated to this patch.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3565
     auto *Bundle =
-        BlocksSchedules[BB]->getScheduleData(isOneOf(S, VL.back()));
+        BlocksSchedules[BB]->getScheduleData(E->isOneOf(E->Scalars.back()));
     if (Bundle && Bundle->isPartOfBundle())
----------------
It would be nice to have a method to hide this. Perhaps E->getSchedulingDataKey() or something along those lines.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4004
       if (IsReorder) {
-        S = getSameOpcode(E->Scalars, E->ReorderIndices.front());
-        VL0 = cast<Instruction>(S.OpValue);
+        InstructionsState S =
+            getSameOpcode(E->Scalars, E->ReorderIndices.front());
----------------
Why do we still have the InstructionsState here? Shouldn't we use data from the TreeEntry instead?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4156
+      assert(E->isAltShuffle() &&
+             ((Instruction::isBinaryOp(E->MainOp->getOpcode()) &&
+               Instruction::isBinaryOp(E->AltOp->getOpcode())) ||
----------------
I think it would be nice to have TreeEntry methods E->getMainOpcode() and E->getAltOpcode().


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

https://reviews.llvm.org/D64700