[PATCH] D78115: [LV] Fix PR45525

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

I think this is related to PR44800, which has similarly redundant phis, but with 2 incoming values from the same block. Relaxing the assertion seems a fine workaround for now, but I think ideally we would eliminate such redundant PHIs in VPlan later on.

LGTM, with some suggestions for the test. It would be good to wait with committing a day or so, in case there are additional comments.



================
Comment at: llvm/test/Transforms/LoopVectorize/pr45525.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -loop-vectorize -S | FileCheck %s
+
----------------
Should this have -force-vector-width=4 or something, to not rely on the cost model?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr45525.ll:44
+bb.1:                                             ; preds = %bb.3, %bb.0
+  %v_loop_1814.0 = phi i32 [ 0, %bb.0 ], [ %inc1290, %bb.3 ]
+  br i1 undef, label %bb.3, label %bb.2
----------------
it would be nice to rename the values to have a bit more compact/descriptive names, e.g. `%iv` and `%iv.next` and so on instead of the longer names.


================
Comment at: llvm/test/Transforms/LoopVectorize/pr45525.ll:45
+  %v_loop_1814.0 = phi i32 [ 0, %bb.0 ], [ %inc1290, %bb.3 ]
+  br i1 undef, label %bb.3, label %bb.2
+
----------------
Branch on undef is UB, please use a different branch condition (e.g. through a parameter).


================
Comment at: llvm/test/Transforms/LoopVectorize/pr45525.ll:48
+bb.2:                                             ; preds = %bb.1
+  %cond9203 = phi i32 [ undef, %bb.1 ]
+  br label %bb.3
----------------
Better to also have a concrete value instead of undef. Also, might be good to have a user of the value to make the test more robust (in case we decide to remove unused values in a VP2VP transform later on).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78115