[PATCH] D136757: [SLP] Extend reordering data of tree entry to support PHI nodes

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3783
     return TE.ReorderIndices;
+  if (TE.State == TreeEntry::Vectorize && (isa<PHINode>(TE.getMainOp()))) {
+    auto PHICompare = [](llvm::Value *V1, llvm::Value *V2) {
----------------
&& TE.getOpcode() == Instruction::PHI


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3785
+    auto PHICompare = [](llvm::Value *V1, llvm::Value *V2) {
+      if (!V1->user_empty() && !V2->user_empty()) {
+        auto *FirstUserOfPhi1 = cast<Instruction>(*V1->user_begin());
----------------
Better to do:
```
if (V1->user_empty() || V2->user_empty())
  return false;
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3788
+        auto *FirstUserOfPhi2 = cast<Instruction>(*V2->user_begin());
+        if (FirstUserOfPhi1->getParent() == FirstUserOfPhi2->getParent()) {
+          if (auto *IE1 = dyn_cast<InsertElementInst>(FirstUserOfPhi1))
----------------
I don't think the parent comparison should be here at all, I think you can drop it.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3791
+            if (auto *IE2 = dyn_cast<InsertElementInst>(FirstUserOfPhi2))
+              return *getInsertIndex(IE1) < *getInsertIndex(IE2);
+
----------------
getInsertIndex may return None, which is not derefereancable (if index is not constant, for example), need to check it to avoid crash.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3793-3795
+          if (auto *EE1 = dyn_cast<ExtractElementInst>(FirstUserOfPhi1))
+            if (auto *EE2 = dyn_cast<ExtractElementInst>(FirstUserOfPhi2))
+              return *getExtractIndex(EE1) < *getExtractIndex(EE2);
----------------
I think you need to check for incoming values here rather than for the users.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3810
+    for (unsigned Id = 0, Sz = TE.Scalars.size(); Id < Sz; ++Id) {
+      PhiToId[TE.Scalars[Id]] = Id;
+      Phis.push_back(TE.Scalars[Id]);
----------------
I think this is dangerous to do, TE may have its reordering order already, you ignore it here.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3813
+    }
+    llvm::sort(Phis, PHICompare);
+    for (unsigned Id = 0, Sz = Phis.size(); Id < Sz; ++Id)
----------------
What if different phis are inserted into the different vectors but using the same indices?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136757