[PATCH] D90562: [VPlan] Use VPDef for VPInterleaveRecipe.

[Gil Rapaport via Phabricator via llvm-commits]

gilr added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7753
 
+  auto skipDeadInterleaveMembers =
+      [&DeadInterleaveGroupMembers](Instruction *I) {
----------------
fhahn wrote:
> gilr wrote:
> > fhahn wrote:
> > > gilr wrote:
> > > > Could you elaborate on why applying the interleave groups is moved back to VPlan creation?
> > > The current handling (create recipes for each instruction and remove & replace them with a single interleave recipe later) seems like a work-around and now that we can manage the VPValues for the members of the interleave groups directly, I think it makes sense to directly create them along with the other recipes.
> > > 
> > > Unfortunately this requires a bit more extra logic to skip interleave groups members when recording the sink-after points, which is not great. But I think this will go away in the future, if we start by building an initial VPlan and gradually lower to specialized recipes.
> > > 
> > > But I can also keep the VPInterleaveRecipe creation at the original position, if you prefer.
> > > Unfortunately this requires a bit more extra logic to skip interleave groups members when recording the sink-after points, which is not great.
> > 
> > Right. This dependence between sink-after and interleave groups which translates to careful bookkeeping is why LV applies them as ordered VPlan-VPlan transformations. Ayal presented this change at the [[ https://youtu.be/BjBSJFzYDVk?t=1326 | 2017 Dev. Meeting ]] as a motivating example for the general direction of an initial trivial VPlan gradually & immediately transformed as decisions are taken, replacing LV's decision-recording maps which make it difficult to compose decisions.
> I have a few patches to get us roughly there and D90712 applies sink-after before lowering to specialized recipes, which makes the recording of recipes redundant. So my preference would be to keep thing as-is in the current patch. But as I said I am more than happy to update the patch to do it the 'old' way for now if you think that's preferable.
>But as I said I am more than happy to update the patch to do it the 'old' way for now if you think that's preferable.

I think so, to avoid the bookkeeping and keep the patch's scope minimal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90562