[PATCH] D136757: [SLP] Extend reordering data of tree entry to support PHI nodes
krishna chaitanya sankisa via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 31 03:20:25 PDT 2022
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
More information about the llvm-commits
mailing list