[PATCH] D116928: [LoopVectorize] Support epilogue vectorisation of loops with reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8269-8271
+  SmallVector<PHINode *, 4> PhisInBlock;
+  for (PHINode &Phi : VecEpilogueIterationCountCheck->phis())
+    PhisInBlock.push_back(&Phi);
----------------
nit: SmallVector has a constructor that takes an iterator range, so I think you can just write:

  SmallVector<PhiNode*> PhisInBlock(VecEpilogueIterationCountCheck->phis());


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10604
+        VPBasicBlock *Header = BestEpiPlan.getEntry()->getEntryBasicBlock();
+        for (VPRecipeBase &R : Header->phis())
+          if (auto *ReductionPhi = dyn_cast<VPReductionPHIRecipe>(&R)) {
----------------
nit: please add braces for the outer for-loop too.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-epilog-vect-inloop-reductions.ll:120
+;
+; Cannot vectorize with scalable vectors (unsupported reduction kind)
+;
----------------
What's the value in testing this? (We already know that for SVE we don't vectorize a mul reduction?)


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-epilog-vect-reductions.ll:242
+;
+; Conditional reduction with interleaving
+;
----------------
Is there anything specifically different about how reductions are handled in epilogue vectorization, that requires this to be tested explicitly? (same for a max-reduction above)


================
Comment at: llvm/test/Transforms/LoopVectorize/epilog-vectorization-reductions.ll:200
+;
+; Integer reduction which requires scalarising as we cannot use gather/scatter
+;
----------------
Again, I'm not entirely sure of the value of testing this case? (as in, how it relates to this patch)


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

https://reviews.llvm.org/D116928