[PATCH] D99354: [SimpleLoopUnswitch] Port partially invariant unswitch from LoopUnswitch to SimpleLoopUnswitch

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D99354#2726745 <https://reviews.llvm.org/D99354#2726745>, @fhahn wrote:

> LGTM, thanks! A few small comments that can be addressed before committing without further review I think.

Thanks @fhahn! After updating code following your comments, I will push this change.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3153
+    if (CurrentLoopValid) {
+      // If the current loop has been unswitched with partially invariant
+      // condition, we should not re-add the current loop to avoid unswitching
----------------
fhahn wrote:
> nit: perhaps `unswitched using a partially invariant condition, ...`?
Yep, I will update it.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/partial-unswitch.ll:1104
+
+define void @test(i32 %0, i32 %1, i32 %2, ...) {
+; CHECK-LABEL: @test(
----------------
fhahn wrote:
> Could you add a brief comment and a more descriptive name for the test?
Yep, I will add them.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/partial-unswitch.ll:1143
+  br label %5
+
+5:
----------------
fhahn wrote:
> It would be great if you could use descriptive labels in the test both for basic blocks and values, to make it easier to read in case people need to take a look in the future.
Yep, I will update it.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/partial-unswitch.ll:1150
+8:
+  store i32 undef, i32* null, align 16
+  br label %9
----------------
fhahn wrote:
> please avoid using `null` as pointer, as this UB
Yep, I will update it.


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

https://reviews.llvm.org/D99354