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.

In D30715#696217, @mkuper wrote:

(If you're not committing the addMetaData fix immediately after this, please put a FIXME above the "if (VF > 1)")

Attaching the rst in pdf form for convenience.

Yes you did, Matt - sorry for the delay. LGTM too :)

Matt, this patch also seems to solve the problem we had with D25333 of how to write non-target-specific cost-model tests. Is that correct?
If so, maybe also move the test added back then out of AArch64? (There are also some other tests I noticed back then currently duplicated for AArch/ARM and X86 that might also be movable to the platform-independent zone).

So committing to a single predicated block may be too aggressive at this point, for example in the following where the srem is moved into the predicated block and above the vectorized sub feeding it: