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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

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?


https://reviews.llvm.org/D38676