[PATCH] D147467: [VPlan] Add VPInterleaveRecipe::NeedsMaskForGaps field (NFCI).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2674
 
   Value *MaskForGaps = nullptr;
+  if (NeedsMaskForGaps) {
----------------
Ayal wrote:
> nit (Independent of this patch): MaskForGaps is set here for loads and later reset for stores. Better to either set it here for both or place this setting inside the handling of loads.
Moved inside the load handling: 3f36b9b456ac4bfa695e253926daa87cd9838550


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9028
+        IG, Recipe->getAddr(), StoredValues, Recipe->getMask(),
+        IG->requiresScalarEpilogue() && !CM.isScalarEpilogueAllowed());
     VPIG->insertBefore(Recipe);
----------------
Ayal wrote:
> nit: may be clearer to first set `bool NeedsMaskForGaps = IG->requiresScalarEpilogue() && !CM.isScalarEpilogueAllowed();` and then use it, instead of setting it inline.
Should be adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1396
 
+  bool NeedsMaskForGaps = false;
+
----------------
Ayal wrote:
> nit: would be good to document and clarify or rename the two masks: `HasMask` indicates if the interleave group is inside a conditional basic block, i.e., HasBlockMask or HasMaskForBlock. `NeedsMaskForGaps` indicates if the interleave group of loads is allowed to speculatively load absent unused members or must avoid doing so by using a mask, i.e., !CanSpeculativelyLoadWithoutMask, where the speculation relies on loading present members on both sides of missing ones - gaps.
Added comments, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147467/new/

https://reviews.llvm.org/D147467