[PATCH] D87679: [LV] Unroll factor is expected to be > 0

[Florian Hahn via Phabricator via llvm-commits]

fhahn requested changes to this revision.
fhahn added a comment.
This revision now requires changes to proceed.

Thanks for the patch! Some comments inline.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5617
   // that the target and trip count allows.
   if (IC > MaxInterleaveCount)
     IC = MaxInterleaveCount;
----------------
IIUC the problem here is in cases when `MaxInterleaveCount == 0`, right? 

The code below expects MaxInterleaveCount > 0 and I think it would be slightly preferable to make sure `MaxInterleaveCount` is at least 1. With that, the code setting `IC` should do the right thing, and other (potential future) users of `MaxInterleaveCount` can expect a sane value.


================
Comment at: llvm/test/Transforms/LoopVectorize/zero_unroll.ll:1
+; RUN: opt -S -loop-vectorize -mtriple=s390x-linux-gnu -tiny-trip-count-interleave-threshold=4 -vectorizer-min-trip-count=8 < %s
+; RUN: opt -S -passes=loop-vectorize -mtriple=s390x-linux-gnu -tiny-trip-count-interleave-threshold=4 -vectorizer-min-trip-count=8 < %s
----------------
I think the test needs moving to `llvm/test/Transforms/LoopVectorize/SystemZ/`, because it relies on SytemZ's cost model and might fail if LLVM is build without the SystemZ backend.

Could you also add a check line to make sure a vector body is generated?


================
Comment at: llvm/test/Transforms/LoopVectorize/zero_unroll.ll:5
+; Function Attrs: nofree norecurse noreturn nounwind writeonly
+define dso_local i32 @main(i32 %arg, i8** nocapture readnone %arg1) local_unnamed_addr #0 {
+bb:
----------------
nit: `dso_local` `local_unnamed_addr` unneeded?


================
Comment at: llvm/test/Transforms/LoopVectorize/zero_unroll.ll:25
+
+attributes #0 = { nofree norecurse noreturn nounwind writeonly "target-cpu"="z13" }
+
----------------
nit: are those attributes needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679