[PATCH] D128033: [LoopVectorize] Fix createInductionResumeValues

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This change looks sensible to me! I just wondered if it's possible to reduce/tidy the test a bit further?



================
Comment at: llvm/test/Transforms/LoopVectorize/create-induction-resume.ll:89
+
+bb8:                                              ; preds = %bb19, %bb6
+  switch i32 undef, label %bb19 [
----------------
Is it possible to simplify this test a little? It's quite difficult to follow what's going on here. It looks like the actual loop only requires bb8, bb9, bb10, bb11, bb18 and bb19.


================
Comment at: llvm/test/Transforms/LoopVectorize/create-induction-resume.ll:90
+bb8:                                              ; preds = %bb19, %bb6
+  switch i32 undef, label %bb19 [
+  i32 8, label %bb10
----------------
nit: Instead of `undef` can you use a real value - perhaps `%arg`?


================
Comment at: llvm/test/Transforms/LoopVectorize/create-induction-resume.ll:105
+bb11:                                             ; preds = %bb11, %bb9
+  %tmp12 = phi i32 [ %tmp14, %bb11 ], [ undef, %bb9 ]
+  %tmp13 = phi i64 [ %tmp16, %bb11 ], [ 1, %bb9 ]
----------------
nit: I think where possible it's good if you can avoid using `undef` and use deterministic values. I imagine you can just replace undef with a constant like 1 and the crash will still occur.


================
Comment at: llvm/test/Transforms/LoopVectorize/create-induction-resume.ll:113
+
+bb18:                                             ; preds = %bb11
+  br label %bb19
----------------
nit: You can delete bb18 entirely and just do 

    br i1 %tmp17, label %bb19, label %bb11

  bb19:                                             ; preds = %bb11, %bb8
    br label %bb8



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

https://reviews.llvm.org/D128033