[PATCH] D105996: [AArch64] Enable Upper bound unrolling universally

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

In D105996#2956941 <https://reviews.llvm.org/D105996#2956941>, @fhahn wrote:

> In D105996#2954149 <https://reviews.llvm.org/D105996#2954149>, @jaykang10 wrote:
>
>> @fhahn Can you share the GeekBench score please? If it is ok, I would like to push this change.
>
> Looks like there are no notable changes to geekbench scores either.
>
> LGTM, which a few nits about the test case

Thanks for checking the geek bench score.

After updating the test, let me push this change.



================
Comment at: llvm/test/Transforms/LoopUnroll/AArch64/unroll-upperbound.ll:4
+
+define void @test(i1 %cond) {
+; CHECK-LABEL: @test(
----------------
fhahn wrote:
> It would be helpful to add a brief comment here to explain what the test is checking.
Yep, let me add a comment.


================
Comment at: llvm/test/Transforms/LoopUnroll/AArch64/unroll-upperbound.ll:42
+  switch i32 %i.017, label %for.body.if.end6_crit_edge [
+  i32 2, label %if.then4
+  ]
----------------
fhahn wrote:
> nit: maybe indent the case?
Yep, let me change it.


================
Comment at: llvm/test/Transforms/LoopUnroll/AArch64/unroll-upperbound.ll:45
+
+for.body.if.end6_crit_edge:
+  br label %if.then4
----------------
fhahn wrote:
> nit: can the name of the block be shortened/improved?
Yep, let me change it.


================
Comment at: llvm/test/Transforms/LoopUnroll/AArch64/unroll-upperbound.ll:46
+for.body.if.end6_crit_edge:
+  br label %if.then4
+
----------------
fhahn wrote:
> might also be good to add a call or something here so the loop has some observable effect
Yep, let me change it.


================
Comment at: llvm/test/Transforms/LoopUnroll/AArch64/unroll-upperbound.ll:48
+
+if.then4:
+  %inc = add nuw nsw i32 %i.017, 1
----------------
fhahn wrote:
> nit: maybe name this block `latch`/`loop.latch` to make it slightly easier to read?
Yep, let me change it.


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

https://reviews.llvm.org/D105996