[PATCH] D70865: [LV] VPValues for memory operation pointers (NFCI)

[Ayal Zaks via Phabricator via llvm-commits]

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