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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 10 inline comments as done.
bmahjour added a comment.

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

> This is a big change, and here are some notes from my first pass reading through this. Some high level questions here, and find some questions inlined:
>
> - Think we need a doc update with the new vectorization skeleton? The picture in the description of this change? Don't know how feasible that is, some ascii art too as comments?

Sure, I had a similar comment in D88819 <https://reviews.llvm.org/D88819>. I find the textual graph horrendous. I'll create a section under https://llvm.org/docs/Vectorizers.html and put the diagram there.

> - Difficult to see for me, but are there tests with Minsize?

Added optsize and minsize tests.

> - Thanks for running the SPEC numbers! Would it not too difficult to run the llvm test suite too? Hopefully that serves 2 purposes: throw some more code at this to test it, and should probably trigger in a few cases.

I've verified that test-suite is functionally clean. The compile-time and performance numbers fluctuate a lot. Is this normal? I reran what appeared to be a few large (20%+) degradations and they were not reproducible. There were upwards of 14% improvement in some tests but I think those are fluctuations also.

> - Given that there are not test changes in this area, it doesn't look like this is changing the tail folding decision making, there is no interaction with that? I.e., haven't checked, but I guess that is performed first as part of the first step, the "normal" inner loop vectorisation. I guess targets that support this, have some interesting decision making to do: tail folding or epilogue vectorisation. But that doesn't seem to be a problem of this patch.

This does not affect tail-folding decision. When tail-folding is requested, no scalar loop is generated so there is no epilogue to vectorize. However, it is possible to tail-fold the epilogue loop. This patch makes it easier to do that in the future.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:944
+/// them in succession from the loop vectorizer planner.
+class InnerMainLoopAndEpilogueVectorizer : public InnerLoopVectorizer {
+public:
----------------
SjoerdMeijer wrote:
> Bikeshedding names: 
> 
> perhaps `InnerMainLoopAndEpilogueVectorizer` -> `InnerLoopAndEpilogueVectorizer` if `Main` doesn't add much here?
sure.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:980
+/// epilogues.
+class IMLAEMainLoop : public InnerMainLoopAndEpilogueVectorizer {
+public:
----------------
SjoerdMeijer wrote:
> And some more bikeshedding:
> 
> Can we make `IMLAEEpilogueLoop` a bit more readable? Same for IMLAEEpilogueLoop below.
Are you referring to their names or their implementation? If the former, would `EpilogueVectorizerMainLoop` and `EpilogueVectorizerEpilogueLoop` be preferred?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1024
+  /// *epilogue loop* strategy (ie the second pass of vplan execution).
+  BasicBlock *createEpilogueVectorizedLoopSkeleton() final override;
+
----------------
fhahn wrote:
> 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`?
Currently only the skeleton code is specialized, but future enhancements will likely necessitate more specialization (eg to support widened induction vars and live-out phis). We could create a `LoopSkeletonCreator`, and modify the `LoopVectorizationPlanner` interfaces to work with it, but I think extending from the `InnerLoopVectorizer` fits better with the current design. Note that `InnerLoopUnroller` also takes a similar approach and extends `InnerLoopVectorizer` while only overriding a couple of functions.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5714
+  // Original loop vector factor should be at least 16.
+  if (MainLoopVF.getFixedValue() < 16)
+    return Result;
----------------
SjoerdMeijer wrote:
> Why is this 16? Do we need to put this "magic constant" behind a target hook? Or calculate this?
This part is copied from D88819. The value 16 is chosen because it catches the x264 opportunity. As I've been saying all along, we need to replace this with a better cost-model. The cost-modeling work is not in the scope of this patch.


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/optimal-epilog-vectorization.ll:6
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
+
----------------
SjoerdMeijer wrote:
> Just checking: what is PPC specific about this test?
The triple is used to make sure ILV finds the loop profitable and chooses a default VF, triggering vectorization of the main loop and its epilogue. Without this we'd have to force a vectorization factor (through hints) for the main loop, however this patch does not vectorize epilogue loops that have user hints on them. It's debatable whether we should vectorize epilogues for hinted loops or not, or whether we need a new pragma, etc. It should be fairly easy to implement whatever we agree on, so I think we should leave those discussions for later and focus on the codegen in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566