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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

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

I've tried both `CTMark` and `TEST_SUITE_BENCHMARKING_ONLY `.



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


================
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:
> 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.
A reasonable cost function can probably be developed with the existing TTI hooks, so a new one may actually not be necessary.  I agree to deferring this to a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566