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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:175
+
+static cl::opt<bool> EnableEpilogueVectorization(
+    "enable-epilogue-vectorization", cl::init(false), cl::Hidden,
----------------
mivnay wrote:
> bmahjour wrote:
> > xbolva00 wrote:
> > > mivnay wrote:
> > > > Why not enable it by default?
> > > +1
> > The reason I'm reluctant to enable it by default is because the heuristic for choosing when to vectorize the epilogue (and by what VF) is quite simplistic and tuned to a specific benchmark, without consideration for the cost of extra branches, code size increase, extra register spills, etc. 
> > My thinking is we can move towards the goal of enabling it by default in smaller steps, by first implementing the transformation (this patch), then improving the cost-model along with performance tuning (future work). What do you think?
> Unless you see some regressions, I think we can enable it by default.
I'm not aware of any functional or performance regressions. Performance results for SPEC2017 x264 should match those from D88819. @mivnay would you mind verifying that on X86 and ARM? I can do a perf run on POWER.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9101
+      // edges from the first pass.
+      LVP.setBestPlan(EPI.EpilogueVF, EPI.EpilogueUF);
+      EPI.MainLoopVF = EPI.EpilogueVF;
----------------
dmgreen wrote:
> I worry this would not work if we have removed the vplan for the different VF's.
Good catch. We need to make sure that the vplan that's chosen for the main loop supports the requested VF for the epilogue loop. I've updated `selectEpilogueVectorizationFactor()` to check for this.


================
Comment at: llvm/test/Transforms/LoopVectorize/optimal-epilog-vectorization.ll:6
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
+
----------------
dmgreen wrote:
> PowerPC test go into LoopVectorize/PowerPC (I think, if it exists).
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89566