[PATCH] D99354: [SimpleLoopUnswitch] Port partially invariant unswitch from LoopUnswitch to SimpleLoopUnswitch
JinGu Kang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 30 02:55:10 PDT 2021
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
More information about the llvm-commits
mailing list