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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

One more round of high-level remarks before I look at some more details.

>> - 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.

Yeah, that could be the case. There are some (micro)benchmarks with a very small execution time, so more susceptible to fluctuations. Have you tried the CTMark  subset and TEST_SUITE_BENCHMARKING_ONLY variable? Anyway, if it is too noisy, it was at least a good testing exercise.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7476
+
+BasicBlock *EpilogueVectorizerMainLoop::createEpilogueVectorizedLoopSkeleton() {
+  MDNode *OrigLoopID = OrigLoop->getLoopID();
----------------
Do we want to refer in a comment here to the doc and skeleton that you've added?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:980
+/// epilogues.
+class IMLAEMainLoop : public InnerMainLoopAndEpilogueVectorizer {
+public:
----------------
bmahjour wrote:
> 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?
Cheers, much more readable IMHO.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5714
+  // Original loop vector factor should be at least 16.
+  if (MainLoopVF.getFixedValue() < 16)
+    return Result;
----------------
bmahjour wrote:
> 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.
I think that's fine, but we should at least hide this 16 behind a TTI hook that e.g. defaults to 16, which should be a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566