[PATCH] D122126: [LoopVectorize] Don't interleave when the number of runtime checks exceeds the threshold

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:322
 
+  bool hasTooManyRuntimeChecks();
+
----------------
It would be good to document the helper. Also, `requiresTooManyRuntimeChecks` may be slightly better, because `has` seems to imply that the checks are already there to me.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10461
+    if (!LVP.hasTooManyRuntimeChecks()) {
+      IC = CM.selectInterleaveCount(VF.Width, *VF.Cost.getValue());
+    }
----------------
This check here should be sufficient, there should be no need to also check in `selectInterleaveCount`.

Could you just move the remark generation & early exit from `::plan` here?

You might want to skip those checks if there's a UserVF or UserIC used, with those I think we should always vectorize if possible. It also might be good to add a check line to your test which forces an interleave count > 1.


================
Comment at: llvm/test/Transforms/LoopVectorize/interleaved-pointer-runtime-check-unprofitable.ll:1
+; RUN: opt -mtriple=powerpc64-unknown-linux-gnu -mcpu=a2 -S -loop-vectorize  < %s -o - | FileCheck %s
+
----------------
As this relies on the PPC cost-model, this needs to go into the `PowerPC` subdirectory.


================
Comment at: llvm/test/Transforms/LoopVectorize/interleaved-pointer-runtime-check-unprofitable.ll:12
+
+define fastcc void @eddy_diff_caleddy_(i64* %wet_cl, i64* %z_e_8676_1962, i32 %ncol.cast.val) {
+L.LB7_2249.preheader:
----------------
nit: can change `%z_e_8676_1962` to `i64` and remove the first load. Also, names could be tidied up a bit.


================
Comment at: llvm/test/Transforms/LoopVectorize/interleaved-pointer-runtime-check-unprofitable.ll:13
+define fastcc void @eddy_diff_caleddy_(i64* %wet_cl, i64* %z_e_8676_1962, i32 %ncol.cast.val) {
+L.LB7_2249.preheader:
+	%0 = load i64, i64* %z_e_8676_1962, align 8
----------------
nit: it would be good to clean up the names of the blocks a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122126