[PATCH] D89566: [LV] Epilogue Vectorization with Optimal Control Flow

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D89566#2373536 <https://reviews.llvm.org/D89566#2373536>, @SjoerdMeijer wrote:

> In D89566#2373443 <https://reviews.llvm.org/D89566#2373443>, @dmgreen wrote:
>
>>> As it stands right now, VPlan can only model control-flow inside a loop. Since epilogue vectorization is concerned with control-flow around (and outside) the loop, there isn't much that can be done today to make the transformation "VPlany". I understand the ultimate goal of vplan is to also model the context around subject loops (eg the entire loop nest or other code surrounding loops). I wondered about whether it's worth to delay this work until that becomes available, but most of the feedback I received was in the direction of let's get it done now and then do it in vplan when it's capable of representing surrounding context.
>>
>> Yeah. I don't think VPlan should slow this down. The problem is that if no-one pushes on vplan to have those extra features, they will never appear :)
>
> FWIW, I agree with both. The more features we add to the vectoriser, the more difficult it will be for VPlan to catch up. At the same time, I don't think there's an initiative to do this, so waiting for VPlan would be unreasonable. Also, epilogue vectorisation has been an outstanding issue for so long, so it is time it gets addressed.

I anticipate that we will be able to eventually model RT check blocks and the CFG around the vector body somehow in VPlan, which should eventually replace the code added here. But that's not going to happen in the near future, so I don't think this should hinder progress here. I think however we should try to structure the code in a way that will not make our life harder than it needs to be going forward.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1024
+  /// *epilogue loop* strategy (ie the second pass of vplan execution).
+  BasicBlock *createEpilogueVectorizedLoopSkeleton() final override;
+
----------------
IIUC we specialize LV so we can override just `createEpilogueVectorizedLoopSkeleton` and `emitMinimumVectorEpilogueIterCountCheck` (which is only used by `createEpilogueVectorizedLoopSkeleton`.

Could we do a more targeted specialization, e.g. by extracting the skeleton creation code into something like `LoopSekletonCreator` and have a regular and epilogue version of that? Or combine it with `EpilogueLoopVectorizationInfo`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5640
 
+bool LoopVectorizationCostModel::isCandidateForEpilogueVectorization(
+    const Loop &L, ElementCount VF) const {
----------------
I think there are some additional tests cases needed to cover all code paths in here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5644
+  // currently unsupported.
+  for (PHINode &Phi : L.getHeader()->phis())
+    if (Legal->isFirstOrderRecurrence(&Phi) || Legal->isReductionVariable(&Phi))
----------------
nit: could use any_of, e.g. `any_of(L.getHeader()->phis(), [this](PHINode &Phi){ return Legal->isFirstOrderRecurrence(&Phi) || Legal->isReductionVariable(&Phi); })`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5664
+  // currently not supported.
+  for (auto &Entry : Legal->getInductionVars())
+    if (!(isScalarAfterVectorization(Entry.first, VF) ||
----------------
nit: use any_of?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566