ping

Applied review comments.
Reused VPWidenMemoryInstructionRecipe's new getMask() in its execute().

Applied review comments.

Applied review comments.

Applied review comments.

Applied review comments.

In D68577#1701381, @rengolin wrote:

So, IIUC, this is changing tryCreateRecipe to move the interleave recipe creation to the caller, buildVPlanWithVPRecipes. The dependencies with the sink values is recorded initially, then the plans are created, then the sinks are applied and, if any, the interleave groups.

• Applied review comment
• Simplified predicate lambda function

No problem :)
LGTM

LGTM!
(One last nitpick: I'd consider letting EnableBackedgeIndexing default to true as TTI's default already disables it for Hexagon and all other targets)

In D54899#1311068, @samparker wrote:

Thanks for taking a look and for the value renamer tip! The test case has now been renamed and reduced.

Test case for the two-combinations bug greatly reduced by bugpoint (thanks, Quentin!) and checks the generated IR instead of LSR's debug prints.

Added a test case to cover the bug introduced by the first patch and fixed in the second.

I am confused. If they don't do (c) how can they fail?

In D51861#1280671, @qcolombet wrote:

Is this covered by an existing test case?
If not, could you add one?

Bug fix: ScalarEvolution::getAddExpr() may modify the vector of SCEVs it is given as argument, causing 2nd formula to be created with only part of the SCEVs in Ops.
This patch fixes this by using a temporary copy of Ops for the 1st formula.

Thanks Quentin!
Regarding effect on compile time please see attached CTMark results on x86.

Ping

Impl. Sanjay comments.
Thanks for taking the time guys!

Impl. Roman's comment - m_Add instead of m_c_Add.

Addressed review comments.

In D38676#929411, @rengolin wrote:

I finished my review, and apart from my two final comments, everything looks fine.

Thanks for the hard work! :)

Rebasing this patch on its preparatory patch D39068.

• Added LIT test case to make sure the new tryToWidenMemory() method takes scalarization decisions into account.
• NFC: move createBlockInMask(), createEdgeMask() to their would-be location in D38676 as part of the Planner's methods to facilitate their review.

In D39068#916829, @rengolin wrote:

Excellent, ok, this patch is looking good to me now. How does this fit into the whole story?

The other review (D38676) still has this patch inside, plus tests. Are you going to clean that up and move the tests here?

It'd be good to have only the refactoring part there, and only the new plans here, with tests.

Addressed review comment - remove the load/store specialized llvm_unreachable from ILV::widenInstruction, load/store now also handled by the switch's default case.

Ping

In D38676#900051, @gilr wrote:

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.

In D38676#899081, @mkuper wrote:

In 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?

Addressed comment: Added a call to Value2VPValue.clear() on ~VPlan().

In D38676#895867, @rengolin wrote:

Would it be possible to split the patch, so that we can review them in a more concise way?

• Adressed comments.
• Fixed bug: tryToWidenMemory() wasn't considering Cost Model's sink-scalar-operands scalarization decisions.

Thanks, Amara!

Hi Matt,

Applied Michael's comment and removed the comment explaining how using the body as header/latch is safe at this point.

Merged with rL297302 & ironed the affected phi-handling comments.

I did have one minor suggestion to avoid some duplicated text in the comments.