[PATCH] D150700: [LV] Stability fix for outerloop vectorization

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:122
   VPRegionBlock *ParentR = nullptr;
-  if (CurrentLoop) {
+  if (CurrentLoop && CurrentLoop->getLoopDepth() >= TheLoop->getLoopDepth()) {
     auto Iter = Loop2Region.insert({CurrentLoop, nullptr});
----------------
I am not sure this check is sufficient, e.g. if `BB` is the header of an another loop with the same nesting level as `TheLoop`. Not sure if that's possible at the moment as a preheader might be required in all cases, still would be good to add a test case.

IIUC the condition we need to check is that `CurrentLoop` is either `TheLoop` or a sub loop  of it; should be sufficient to check if `TheLoop->contains(CurrentLoop)`. I think this only checks one level, but all loops we currently support should be an inner loop with its outer loop. Could also have an assertion to ensure that (e.g. walk up through `CurrentLoop`'s parents and check that not more than 1 step is needed to reach `TheLoop`.


================
Comment at: llvm/test/Transforms/LoopVectorize/outer_loop_test3.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S -passes=loop-vectorize -enable-vplan-native-path < %s -S -pass-remarks=loop-vectorize 2>%t | FileCheck %s
+
----------------
`-pass-remarks=loop-vectorize` not needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/outer_loop_test3.ll:17
+
+define void @test(i64 %n, ptr %a) {
+; CHECK-LABEL: define void @test
----------------
would be good to have a more descriptive name & a comment with a brief description of what this tests. Test name could also include more info.


================
Comment at: llvm/test/Transforms/LoopVectorize/outer_loop_test3.ll:96
+  %cmp34 = icmp sgt i64 %n, 0
+  br i1 %cmp34, label %for.body.us.preheader, label %for.cond.cleanup
+
----------------
should not be needed


================
Comment at: llvm/test/Transforms/LoopVectorize/outer_loop_test3.ll:101
+
+for.body.us:
+  %indvars.iv42 = phi i64 [ 0, %for.body.us.preheader ], [ %indvars.iv.next43, %for.cond2.for.cond.cleanup4_crit_edge.split.us.us ]
----------------
Could you rename the labels & value names to be more descriptive (and shorter)? e.g. something like  `loop.1.header`, `loop.1.latch`, `iv.1` and so on.


================
Comment at: llvm/test/Transforms/LoopVectorize/outer_loop_test3.ll:106
+  store i32 0, ptr %0, align 4
+  %1 = trunc i64 %indvars.iv42 to i32
+  %2 = add i32 %1, 2
----------------
should not be needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150700