[PATCH] D50665: [LV][LAA] Vectorize loop invariant values stored into loop invariant address

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In https://reviews.llvm.org/D50665#1209958, @Ayal wrote:

> In https://reviews.llvm.org/D50665#1208026, @anna wrote:
>
> > In https://reviews.llvm.org/D50665#1206780, @Ayal wrote:
> >
> > > In https://reviews.llvm.org/D50665#1200509, @anna wrote:
> > >
> > > >
> > >
> > >
> > > ...
> > >
> > > > Yes, the stores are scalarized. Identical replicas left as-is. Either passes such as load elimination can remove it, or we can clean it up in LV itself.
> > >
> > >
> > >
> > > - - by revisiting LoopVectorizationCostModel::collectLoopUniforms()?  ;-)
> >
> >
> > Right now, I just run instcombine after loop vectorization to clean up those unnecessary stores (and test cases make sure there's only one store left). Looks like there are other places in LV which relies on InstCombine as the clean up pass, so it may not be that bad after all? Thoughts?
>
>
> Yeah, this is a bit embarrassing, but currently invariant loads also get replicated (and cleaned up later), despite trying to avoid doing so by recording IsUniform in VPReplicateRecipe. In general, if it's simpler and more consistent to generate code in a common template and potentially cleanup later, should be ok provided the cost model accounts for it accurately and cleanup is guaranteed, as checked by tests. BTW, LV already has an internal cse(). But in this case, VPlan should reflect the final outcome better, i.e., with a correct IsUniform. This should be taken care of, possibly by a separate patch.


I see. thanks for the clarification. So, for now, I'll leave the stores in the IR just like we're doing for the loads and add a "TODO" for both.


Repository:
  rL LLVM

https://reviews.llvm.org/D50665