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
485–498

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

2205

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
485–498

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

2205

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
484–485

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

484–485

ditto (both comment and BlockInMask).

485

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

2191–2192

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

2209

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

2219–2220

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

2369

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

6803

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
746

(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
277

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

892

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

899

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

1041

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

1048

// 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
2369

This method should no longer be called to vectorize interleave groups

Right.

Unrelated, can be committed separately.

Agreed.

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

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
485–498

("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.)

2189

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).

2229

PtrTy >> AddrTy?

2369

Can leave behind a FIXME

2394–2395

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

2400

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

2433

(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.