[PATCH] D70865: [LV] VPValues for memory operation pointers (NFCI)
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 26 04:00:16 PST 2019
Ayal added a comment.
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(?).
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:486
/// Try to vectorize the interleaved access group that \p Instr belongs to,
/// optionally masking the vector operations if \p BlockInMask is non-null.
+ void vectorizeInterleaveGroup(Instruction *Instr, VPTransformState &State,
----------------
Add to comment what \p State and \p Addr are used for.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:488
+ void vectorizeInterleaveGroup(Instruction *Instr, VPTransformState &State,
+ VPValue *Addr, VPValue *BlockInMask = nullptr);
----------------
Unrelated, but better use "Mask" than "BlockInMask" here and elsewhere?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:491
/// Vectorize Load and Store instructions, optionally masking the vector
/// operations if \p BlockInMask is non-null.
+ void vectorizeMemoryInstruction(Instruction *Instr, VPTransformState &State,
----------------
ditto (both comment and BlockInMask).
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2187
- VectorParts Mask;
+ VectorParts Mask(UF);
bool IsMaskForCondRequired = BlockInMask;
----------------
("BlockInMask" is the VPValue, "Mask" is the corresponding code-gen'd Value(s).)
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2210
+
+ bool InBounds = false;
+ if (auto *gep = dyn_cast<GetElementPtrInst>(NewPtr->stripPointerCasts()))
----------------
Having sunk InBounds into for for-Part loop, better place it closer to its usage below.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2226
NewPtr = Builder.CreateGEP(ScalarTy, NewPtr, Builder.getInt32(-Index));
if (InBounds)
cast<GetElementPtrInst>(NewPtr)->setIsInBounds(true);
----------------
(It's probably better to then call CreateInBoundsGEP(); or always setIsInBounds(InBounds) similar to vectorizeMemoryInstruction() below, but that's unrelated.)
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2369
if (Decision == LoopVectorizationCostModel::CM_Interleave)
- return vectorizeInterleaveGroup(Instr);
+ return vectorizeInterleaveGroup(Instr, State, Addr, BlockInMask);
----------------
Replace with assert? This method should no longer be called to vectorize interleave groups. Unrelated, can be committed separately.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6764
+ Value *Addr = getLoadStorePointerOperand(I);
+ assert(Addr && "Expected a load/store at this point");
+
----------------
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?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:740
+ getAddr()->printAsOperand(O);
+ VPValue *Mask = getMask();
+ if (Mask) {
----------------
(This change replacing User->getOperand(0) with getMask(), here and in VPInterleaveGroupRecipe::print() above, belonged to D68577; can commit separately)
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:277
+ /// this method will delegate the call to ILV in such cases in order to
+ /// provide callers a consistent API.
+ Value *get(VPValue *Def, const VPIteration &Instance) {
----------------
Should above comment be updated, given that this method (currently) always delegates the call to ILV?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:894
+ VPValue *getAddr() const {
+ // Address is the 1st operand.
+ return User.getOperand(0);
----------------
// Address is the 1st[, mandatory] operand.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:901
+ VPValue *getMask() const {
+ // Mask is the 2nd operand.
+ return User.getNumOperands() == 2 ? User.getOperand(1) : nullptr;
----------------
// Mask is the 2nd[, optional, last] operand.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1044
+ VPValue *getAddr() const {
+ // Address is the 1st operand.
+ return User.getOperand(0);
----------------
// Address is the 1st[, mandatory] operand.
(Implementation can be inline.)
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1051
+ VPValue *getMask() const {
+ // Mask is the 2nd operand.
+ return User.getNumOperands() == 2 ? User.getOperand(1) : nullptr;
----------------
// Mask is the 2nd[, optional, last] operand.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70865/new/
https://reviews.llvm.org/D70865
More information about the llvm-commits
mailing list