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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. | |
1048 | // Mask is the 2nd[, optional, last] operand. |
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);) |
Add to comment what \p State and \p Addr are used for.