[PATCH] D38676: [LV] Model masking in VPlan, introducing VPInstructions

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In https://reviews.llvm.org/D38676#899081, @mkuper wrote:

> In https://reviews.llvm.org/D38676#898099, @aemerson wrote:
>
> > A minor nit but LGTM. I don't have an aversion to "dead code" if it's going to be used in the near future, so perhaps to make the patch smaller split it into an initial patch to add VPlanValue.h and then the VPlanBuilder.h as well as the changes to the documentation. Those pieces seem uncontroversial to me, before moving onto the vectorizer. @mkuper does this make sense?
>
>
> My $0.02:
>  Basically, I agree with @aemerson  and @rengolin, at least to the extent that even if either way could work, it would be best to do it the way that ends up with simpler patches and will take less time (overall) to review well.
>
> I don't think there's a problem of dead code, or "it's unclear how the infrastructure will be used", since you already have the code that actually uses it ready for review as well. So, it would probably best to split it into two dependent patches, and post them for review separately. Then, the second one can be rebased on top of the first one, if it has any significant changes, when it goes in. Admittedly, it's more work, but I think it's worth it.
>
> WDYT?


Yes, small & digestible patches are ideal. So, as you guys are all Ok with introducing the VPInstruction infrastructure w/o its use, let's go with that.
One change we can probably peel off before, though, is to first introduce VPBlendRecipe and VPWidenMemoryInstruction w/o the new masking code. This should later narrow the diff in LoopVectorize.cpp and reflect the masking changes on those Recipes as well.


https://reviews.llvm.org/D38676