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

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added inline comments.


================
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))
----------------
ABataev wrote:
> I don't think the parent comparison should be here at all, I think you can drop it.
Removed it.


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


================
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);
----------------
ABataev wrote:
> I think you need to check for incoming values here rather than for the users.
In this patch I'm trying to reorder phis based on result use order(extractelement/insertelement order). Is there any check needed for  incoming values of phis here?


================
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]);
----------------
ABataev wrote:
> I think this is dangerous to do, TE may have its reordering order already, you ignore it here.
added check to see that reordering order is empty. TE.ReorderIndices.empty()


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3813
+    }
+    llvm::sort(Phis, PHICompare);
+    for (unsigned Id = 0, Sz = Phis.size(); Id < Sz; ++Id)
----------------
ABataev wrote:
> What if different phis are inserted into the different vectors but using the same indices?
For insertelements, areTwoInsertFromSameBuildVector() is used to check the inserts are to same vector. For extractelements, check is done to see that extract is from same vector.


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

https://reviews.llvm.org/D136757