[PATCH] D98714: [SLP] Add insertelement instructions to vectorizable tree

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2878-2879
+        VL0 = cast<Instruction>(VL[0]);
+        Bundle = BS.tryScheduleBundle(VL, this, S);
+        assert(Bundle && "Bundle isn't scheduled after reordering?");
+      }
----------------
ABataev wrote:
> ABataev wrote:
> > anton-afanasyev wrote:
> > > ABataev wrote:
> > > > Why do we need to do this tricky stuff?
> > > Why do we need to rearrange `VL`? We make def-use order for this to correctly extract scalars used out-of-tree.
> > > 
> > > We can get rid of rescheduling here by two ways:
> > > 1) Sort `VL` _before_ scheduling, at the beginning of `buildTree_rec()`;
> > > 2) Don't sort `VL` at all (use index order made by `findBuildAggregate()` and recover this order every time again when needed.
> > > 
> > > I'm to try 2) way here.
> > Why do you need to reschedule the bundle?
> Can you try to sort the VL before calling the buildtree function? To avoid rescheduling?
No need to reschedule actually but to set some of variables to sync bundle with tree entry. But I've reworked this again.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2878-2879
+        VL0 = cast<Instruction>(VL[0]);
+        Bundle = BS.tryScheduleBundle(VL, this, S);
+        assert(Bundle && "Bundle isn't scheduled after reordering?");
+      }
----------------
anton-afanasyev wrote:
> ABataev wrote:
> > ABataev wrote:
> > > anton-afanasyev wrote:
> > > > ABataev wrote:
> > > > > Why do we need to do this tricky stuff?
> > > > Why do we need to rearrange `VL`? We make def-use order for this to correctly extract scalars used out-of-tree.
> > > > 
> > > > We can get rid of rescheduling here by two ways:
> > > > 1) Sort `VL` _before_ scheduling, at the beginning of `buildTree_rec()`;
> > > > 2) Don't sort `VL` at all (use index order made by `findBuildAggregate()` and recover this order every time again when needed.
> > > > 
> > > > I'm to try 2) way here.
> > > Why do you need to reschedule the bundle?
> > Can you try to sort the VL before calling the buildtree function? To avoid rescheduling?
> No need to reschedule actually but to set some of variables to sync bundle with tree entry. But I've reworked this again.
I've chosen the 2) way eventually, it looks more simple. No need to reorder and to use `ReorderIndicies`. The only disadvantage -- need to look for `FirstInsert` several times redundantly  (up to O(n^2), but linear for common cases).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98714