[PATCH] D142258: [LV] Ignore runtime checks threshold when vectorization is forced

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the patch! Add some suggestion on how to simplify the tests a bit. Also, could you pre-commit the simplified tests independent of the patch and then update the revision here to just show the difference caused by the patch?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1919
     // Hard cutoff to limit compile-time increase in case a very large number of
-    // runtime checks needs to be generated.
+    // runtime checks needs to be generated, unless user forced vectorization
     // TODO: Skip cutoff if the loop is guaranteed to execute, e.g. due to
----------------
nit: period at end of sentence.


================
Comment at: llvm/test/Transforms/LoopVectorize/memcheck_threashold_vec_forced.ll:6
+
+; CHECK-DEBUG-LABEL: LV: Checking a loop in 'vec_forced'
+; CHECK-DEBUG: Executing best plan with VF=4,
----------------
Given that the test checks the full IR, it would probably be better to remove the check of debug output and run it unconditionally (remove `REQUIRES: asserts`).


================
Comment at: llvm/test/Transforms/LoopVectorize/memcheck_threashold_vec_forced.ll:63
+entry:
+  %cmp6 = icmp sgt i32 %n, 0
+  br i1 %cmp6, label %for.body.preheader, label %for.cond.cleanup
----------------
the check shouldn't be needed.


================
Comment at: llvm/test/Transforms/LoopVectorize/memcheck_threashold_vec_forced.ll:67
+for.body.preheader:
+  %wide.trip.count = zext i32 %n to i64
+  br label %for.body
----------------
can just pass `%n` as `i64` and get rid of the zext.


================
Comment at: llvm/test/Transforms/LoopVectorize/memcheck_threashold_vec_forced.ll:71
+for.cond.cleanup.loopexit:
+  br label %for.cond.cleanup
+
----------------
can directly return here and maybe move the exit block to the end of the function.


================
Comment at: llvm/test/Transforms/LoopVectorize/memcheck_threashold_vec_forced.ll:77
+for.body:
+  %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds i32, ptr %b, i64 %indvars.iv
----------------
can drop `indvars.` prefix to make value names more compact and readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142258