This is an archive of the discontinued LLVM Phabricator instance.

[LV] VPValues for memory operation pointers (NFCI)
ClosedPublic

Authored by gilr on Nov 29 2019, 11:50 AM.

Details

Summary

Memory instruction widening recipes use the pointer operand of their load/store ingredient for generating the needed GEPs, making it difficult to feed these
recipes with pointers based on other ingredients or none at all.
This patch modifies these recipes to use a VPValue for the pointer instead, in order to reduce ingredient def-use usage by ILV as a step towards full VPlan-based def-use relations. The recipes are constructed with VPValues bound to these ingredients, maintaining current behavior.

Diff Detail

Event Timeline

gilr created this revision.Nov 29 2019, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2019, 11:50 AM

As far as I can see, this shouldn't introduce behavioural changes and does add some nice cleanups.

But I'd be more comfortable with some other pair of eyes looking over it, as I'm rusty with VPlan code.

Also, some nits inline.

Thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
487

nit: keep the same formatting as above (or vice versa)

2201

What's the difference between UF and State.UF? This looks confusing now.

gilr marked 2 inline comments as done.Dec 9 2019, 12:18 PM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
487

Not sure I follow ... both vectorizeInterleaveGroup and vectorizeMemoryInstruction were formatted by clang-format.

2201

Right. Will switch to LV's UF for consistency.

gilr updated this revision to Diff 232910.Dec 9 2019, 12:29 PM

Addressed comments.

Ayal added a comment.Dec 26 2019, 3:57 AM

Looks good to me; important step forward teaching ILV to deal with VPValues, lifting dependencies on ingredient operands and other attributes, simplifying recipes.

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

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
481–482

Add to comment what \p State and \p Addr are used for.

483

Unrelated, but better use "Mask" than "BlockInMask" here and elsewhere?

486–487

ditto (both comment and BlockInMask).

2182

("BlockInMask" is the VPValue, "Mask" is the corresponding code-gen'd Value(s).)

2205

Having sunk InBounds into for for-Part loop, better place it closer to its usage below.

2221

(It's probably better to then call CreateInBoundsGEP(); or always setIsInBounds(InBounds) similar to vectorizeMemoryInstruction() below, but that's unrelated.)

2364

Replace with assert? This method should no longer be called to vectorize interleave groups. Unrelated, can be committed separately.

6743

Might be slightly confusing to use Addr for both Value and VPValue, plus assert is somewhat redundant given the above isa's and assert of getOrAddVPValue() below. How about doing:

VPValue *Addr = Plan->getOrAddVPValue(getLoadStorePointerOperand(I));

, as done in VPInstructionsToVPRecipes() below?

llvm/lib/Transforms/Vectorize/VPlan.cpp
726

(This change replacing User->getOperand(0) with getMask(), here and in VPInterleaveGroupRecipe::print() above, belonged to D68577; can commit separately)

llvm/lib/Transforms/Vectorize/VPlan.h
276

Should above comment be updated, given that this method (currently) always delegates the call to ILV?

863

// Address is the 1st[, mandatory] operand.

870

// Mask is the 2nd[, optional, last] operand.

1013

// Address is the 1st[, mandatory] operand.
(Implementation can be inline.)

1020

// Mask is the 2nd[, optional, last] operand.

gilr marked 13 inline comments as done.Dec 28 2019, 9:57 AM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2364

This method should no longer be called to vectorize interleave groups

Right.

Unrelated, can be committed separately.

Agreed.

llvm/lib/Transforms/Vectorize/VPlan.cpp
726

Separated into D71964

gilr updated this revision to Diff 235500.Dec 29 2019, 1:02 AM

Addressed comments.

gilr added a comment.Jan 8 2020, 4:38 AM

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

Right. Will unify names and add a "Part" suffix to the per-part IR values.

gilr updated this revision to Diff 236794.Jan 8 2020, 4:41 AM

Addressed comments.

Ayal accepted this revision.Jan 8 2020, 11:57 PM

This looks good to me, ship it. Added some final minor optional nits.

BlockInMask helps distinguish between the mask of a conditional block and the mask of an interleave group with gaps.

A VPValue variable covers all unroll parts, whereas an IR Value variable generated for it is per unroll part. So the naming convention can use Addr, BlockInMask for VPValue's and AddrPart, BlockInMaskPart for their corresponding IR Value's.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
487–489

("Try to vectorize" - indeed this actually does "try to", as it ends up not vectorizing the interleave group when Instr is not the insert position.)

2179

May be better to place the definition of AddrParts with the comment, next to the loop below that prepares for the new pointers/addresses (Ptr >> Addr).

2228

PtrTy >> AddrTy?

2364

Can leave behind a FIXME

2389–2390

Fold as done above to if (BlockInMask) ? Independent change, can be done separately.

2395

To match the above naming, both Ptr and PartPtr should be renamed AddrPart? NFC, possibly done separately(?)

2428

(In a future step this too should turn into State.get(Val, Part);)

This revision is now accepted and ready to land.Jan 8 2020, 11:57 PM
This revision was automatically updated to reflect the committed changes.