[PATCH] D23410: [SLP] Initialize VectorizedValue when gathering

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2175
@@ +2174,3 @@
+  auto ForwardStart = BasicBlock::iterator(Leader);
+  for (auto &I : make_range(ForwardStart, BB->end())) {
+    if (Bundle.erase(&I))
----------------
mssimpso wrote:
> mkuper wrote:
> > Just to make sure I understand this correctly - you're running from Leader, inclusive, so that if VL[0] is the last instruction in program order, you'll end up with LastInst == Leader, right?
> > If so, it's probably worth a comment - or just initialize LastInst to Leader instead of null.
> > 
> > Also, won't we get worst-case quardatic behavior here, if VL[0] is consistently last (so we end up running to the end of the block for each bundle)?
> Right, if VL[0] is consistently last, we'll end up running to the end of the block a lot. I think the common case here should be that VL[0] is first in program order? (But I could be wrong about this). Is there a better way to do this? setInsertPointAfterBundle doesn't really work as expected.
> 
> For this particular bug, another way to fix the insertion issue might be to modify Gather to change the insert point before generating each insertlement instruction. What do you think?
> 
> 
I agree, the common case is that VL[0] is first - but there are at least two cases where it may be last:
1) For reductions, we may end up collecting things in reverse order (that is, VL[0] ought to be first, but it ends up last).
2) If the root if a bundle of stores, we'll sort the bundle so that consecutive stores in the bundle have consecutive pointers.
And changing the order of the root bundle affects the order of all bundles in the tree, since the lanes have to be consistent, so having things in reversed order propagates.

I think changing the insertion point before generating each insertelement instruction doesn't really solve the problem even for gathers, because the inserts need to be in the bundle order (they form a use-def chain), so we can't just put every insert after its VL[i].

I don't have a good solution off the top of my head, though.


https://reviews.llvm.org/D23410