[PATCH] D136544: [SLP] For vectorizing chains in basic block, decide order of PHI nodes based on their result use.

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added inline comments.


================
Comment at: llvm/test/Transforms/SLPVectorizer/AMDGPU/phi-result-use-order.ll:76
 ; CHECK-NEXT:    [[B3:%.*]] = extractelement <4 x half> [[IN2]], i64 3
-; CHECK-NEXT:    [[TMP4:%.*]] = insertelement <2 x half> poison, half [[B1]], i32 0
-; CHECK-NEXT:    [[TMP5:%.*]] = insertelement <2 x half> [[TMP4]], half [[B0]], i32 1
+; CHECK-NEXT:    [[TMP4:%.*]] = insertelement <2 x half> poison, half [[B0]], i32 0
+; CHECK-NEXT:    [[TMP5:%.*]] = insertelement <2 x half> [[TMP4]], half [[B1]], i32 1
----------------
ABataev wrote:
> skc7 wrote:
> > ABataev wrote:
> > > skc7 wrote:
> > > > ABataev wrote:
> > > > > Do I understand it correct that you're trying to avoid reversing here and above?
> > > > Yes, the order here is dependent on the order of phinodes currently. 
> > > Then you need to do a bit different thing than you're doing. You need to extend the analysis in  BoUpSLP::getReorderingData to check that the buildvector sequence is built in the identity order.
> > Sorting of phis happen using PHICompare before building the tree and entries. Do we need to extend getReorderingData() for phis? And again do the analysis on the initially sorted phis from the tree entry.
> It is not sorting, I would call it rather selection of the most profitable ones.
> The actual reordering of the built graph happens in BoUpSLP::reorderTopToBottom and BoUpSLP::reorderBottomToTop. Yes, you need to extend getReorderingData() to support phis (check the incoming values/users for best order if they are not checked yet.). But you need to check the extractelement/insertelement indices rather than the order of the users.
Thanks @ABataev for review. I have created D136757 to extend getReorderingData() for phis.


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

https://reviews.llvm.org/D136544