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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

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?
- Difficult to see for me, but are there tests with Minsize?
- 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.
- 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.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:177
+    "enable-epilogue-vectorization", cl::init(true), cl::Hidden,
+    cl::desc("Enable vectorization of residual loops."));
+
----------------
nit: for consistency residual -> epilogue?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:944
+/// them in succession from the loop vectorizer planner.
+class InnerMainLoopAndEpilogueVectorizer : public InnerLoopVectorizer {
+public:
----------------
Bikeshedding names: 

perhaps `InnerMainLoopAndEpilogueVectorizer` -> `InnerLoopAndEpilogueVectorizer` if `Main` doesn't add much here?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:980
+/// epilogues.
+class IMLAEMainLoop : public InnerMainLoopAndEpilogueVectorizer {
+public:
----------------
And some more bikeshedding:

Can we make `IMLAEEpilogueLoop` a bit more readable? Same for IMLAEEpilogueLoop below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5714
+  // Original loop vector factor should be at least 16.
+  if (MainLoopVF.getFixedValue() < 16)
+    return Result;
----------------
Why is this 16? Do we need to put this "magic constant" behind a target hook? Or calculate this?


================
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"
+
----------------
Just checking: what is PPC specific about this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566