In D80220#2078460, @fhahn wrote:

In D80220#2078222, @gilr wrote:

We still use GEP->clone() for the case where the GEP is loop invariant. It would probably be best to do that in a follow-on patch. Same for perhaps storing InBounds directly in the recipe as well.

The patch LGTM, any reason not to include the base pointer?

LGTM with a nit. Thanks!

Thanks for looking into this, Florian (and sorry for taking so long).
LGTM, except VPInterleaveRecipe::print() is missing here.
The patch stands by itself as a standardization of printing, similar to how the multi-line Function and Module print. If meant as a first step, would be good to elaborate on the full picture.

LGTM (with that missing comment nit), thanks!

LGTM, thanks.

Addressed comments and narrowed LIT checks to the relevant vectorized instructions.
Thanks @fhahn !

LGTM, thanks!

Addressed comments.

Address comments

LGTM, thanks!

LGTM, with a nit.

Addressed comments.

In D76373#1939483, @fhahn wrote:

LGTM, thanks! Please wait a day or so with committing, in case there are other comments.

Addressed comments.

Ping :)

Clang format.

In D74695#1923526, @fhahn wrote:

I think doing so for VPWidenRecipe might be more tricky than other recipes, as VPWidenRecipe currently contains a list of instructions.
[snip]
I thought a little bit about how to migrate the def-use dependencies for VPWidenRecipe without breaking it up, but couldn't come up with a more straight-forward approach.

LGTM!

In D73078#1903249, @fhahn wrote:

In D73078#1903177, @gilr wrote:

In D73078#1902767, @fhahn wrote:

In D73078#1902664, @gilr wrote:

[snip]
I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?

After D74445 all VPInstructions can indeed print themselves by either using their IR name or defaulting to SlotTracker/badref. So this patch can be simplified by not propagating the SlotTracker through the print() methods, since non-VPInstruction VPValues can rely on their underlying IR values for printing. Note that for this to work addVPValue must plant the underlying IR value into the generated VPValue as you do in D74695.
BackedgeTakenCount would then remain the only user of the current pointer-based naming and consequently of the "where:" clause.

Ah I think I got what you are proposing now: just remove the ::print(raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker) variants taking a SlotTracker, because we can just instantiate it on demand, by just getting the parent plan for the containing VPBB. This means we might have to re-number the plan multiple times when printing the whole plan through VPlanPrinter, but as this is just for debugging (and the scope of a plan should be relatively small) we can adjust that as follow-up, if required.

So the recursive print of any VP entity should use a single, up-to-date SlotTracker. This SlotTracker may be generated/assigned when entering the recursion (e.g. on operator<< or print()) and destroyed/invalidated when leaving it. One could optimize later to invalidate only when needed, i.e. on VPlan changes.

I think I am missing something.

I think this is exactly what the patch does: when calling print without SlotTracker, it will be created on demand and VPlanPrinter re-uses the same slot tracker for printing all VP entities by passing it to print(). I'm not sure what I should change, besides a follow-up to use the underlying values, if available/

In D73078#1902767, @fhahn wrote:

In D73078#1902664, @gilr wrote:

[snip]
I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?

After D74445 all VPInstructions can indeed print themselves by either using their IR name or defaulting to SlotTracker/badref. So this patch can be simplified by not propagating the SlotTracker through the print() methods, since non-VPInstruction VPValues can rely on their underlying IR values for printing. Note that for this to work addVPValue must plant the underlying IR value into the generated VPValue as you do in D74695.
BackedgeTakenCount would then remain the only user of the current pointer-based naming and consequently of the "where:" clause.

Ah I think I got what you are proposing now: just remove the ::print(raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker) variants taking a SlotTracker, because we can just instantiate it on demand, by just getting the parent plan for the containing VPBB. This means we might have to re-number the plan multiple times when printing the whole plan through VPlanPrinter, but as this is just for debugging (and the scope of a plan should be relatively small) we can adjust that as follow-up, if required.

In D73078#1900122, @fhahn wrote:

Updated patch to use getPlan(), add unit tests.

In D73078#1899992, @gilr wrote:

In D73078#1870844, @fhahn wrote:

[snip]
I've put up a patch to add a pointer to VPlan: D74445. I;ve also added implementations of print() with and without SlotTracker (so we can re-use the existing tracker, e.g. from VPlanPrinter. I still need to update the slot tracker to actually use it. VPInstructions without a block (or a VPBasicBlock without parentPlan) should probably be handled similar to IR instructions without BB, by using something like 'badref'.

What do you think?

This added pointer to VPlan can indeed take care of enumerating VPInstructions, defaulting to 'badref'.
Essentially all other VPValues except BackedgeTakenCount are wrappers of underlying IR values (right?), so they can print that value e.g. with the proposed %ir. w/o needing SlotTracker's services, thereby simplifying the patch. This would leave the somewhat special BackedgeTakenCount as the only user of the "where:" clause and current pointer-based naming.

I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?

LGTM, with minor fix.

In D73078#1870844, @fhahn wrote:

[snip]
I've put up a patch to add a pointer to VPlan: D74445. I;ve also added implementations of print() with and without SlotTracker (so we can re-use the existing tracker, e.g. from VPlanPrinter. I still need to update the slot tracker to actually use it. VPInstructions without a block (or a VPBasicBlock without parentPlan) should probably be handled similar to IR instructions without BB, by using something like 'badref'.

What do you think?

An alternative to adding a VPlan* to every VP block might be to reuse their existing Parent property by having (only) the topmost block's parent point to VPlan, similar to the IR hierarchy where ancestors are reached indirectly through transitivity. This would require VPlan to inherit from VPRegion, which may deserve further discussion.
For now let's go ahead with the suggested VPlan pointer in VPBlockBase, but better just call it get/Plan instead of get/ParentPlan, saving parenthood's existing semantics.
Since the only current use case for this is debugging, we can simplify the patch and maintain a future trivial switch to the above by setting only the entry block's VPlan e.g. at VPlan::setEntry() and have getPlan() traverse upwards until reaching a topmost block and backwards until reaching the entry. This also better supports potential moving blocks in and out of VPlans.
Makes sense?

In D73078#1855736, @fhahn wrote:

In D73078#1854151, @gilr wrote:

It would indeed be good to improve VPValue printing. Initially, there were only a handful of VPValues so those pointer-based IDs seemed sufficient.
Since VPValues are now both more common and optionally have an underlying IR value, retaining names from the underlying IR where available could go a long way w.r.t both readability and relating to the loop being vectorized; This would hopefully cover most VPValues.

Sounds good, printing the name of the underlying value should be quite trivial.

BTW, VP basic blocks could also retain the name of the corresponding IR basic block.

Sounds good!

It would also be good to distinguish between "pure" VPValues and VPValues wrapping unnamed IR values (can we somehow retain the latter's enumeration?)

+1. I think we would still need a mechanism like this patch to provide a better numbering for 'pure' VPValues.

It would indeed be good to improve VPValue printing. Initially, there were only a handful of VPValues so those pointer-based IDs seemed sufficient.
Since VPValues are now both more common and optionally have an underlying IR value, retaining names from the underlying IR where available could go a long way w.r.t both readability and relating to the loop being vectorized; This would hopefully cover most VPValues.
BTW, VP basic blocks could also retain the name of the corresponding IR basic block.
It would also be good to distinguish between "pure" VPValues and VPValues wrapping unnamed IR values (can we somehow retain the latter's enumeration?)

LGTM!

In D70865#1796496, @Ayal wrote:

Adding minor comments; may be good to have consistent naming of VPValue variables and their corresponding code-gen'd Values(?).

Addressed comments.

Addressed comments.

In D71964#1798095, @fhahn wrote:

LGTM

Addressed comments.

In D70734#1765693, @fhahn wrote:

In D70734#1764103, @gilr wrote:

Traversing VPRegions is not supported at the moment but we do not create such plans at the moment as far as I know

I assume you're referring to the native path? (non-native generates regions for scalarized & predicated instructions)

Ah right, I missed that code path! For my initial use-case (dropping assumes), the iterator needs to traverse the regions as well then. Let's see if I can get the iterators to co-operate. Do you think it's worth to get the existing iterators in as is? There are a few places that could use them, I think.

LGTM!

Addressed final comment (thanks Florian!)

Addressed comments.

A more comprehensive solution for breaking this ingredient def-use dependence is implemented in D70865.

Traversing VPRegions is not supported at the moment but we do not create such plans at the moment as far as I know

• Limit this commit to handling GEP widening only, per Ayal's comment.
• Bug fix: The all-operands-are-invariant check was not checking the pointer's invariance.

Originally part of D69067, to be reviewed separately per Ayal's comment.

In D68577#1739605, @efriedma wrote:

Testcase follows; reproduce with opt -loop-vectorize:

In D68577#1739177, @efriedma wrote:

Please let me know if you need me to come up with a testcase.

In D68577#1738622, @fhahn wrote:

Gil, I can put a patch up for not sinking past terminators in OVDescriptor, as I am already working on patches in that area.

I’d also suggest to land the removeFromParent & co changes separately from the other changes in the patch.

In D68577#1732352, @bkramer wrote:

This fails with asan: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/36343/steps/check-llvm%20asan/logs/stdio
Reverted in d3ec06d219788801380af1948c7f7ef9d3c6100b

Addressed comments.

Thanks everyone!

Address comments.