[PATCH] D97947: [AArch64] Force runtime unrolling for in-order scheduling models

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1110
+
+  // Scan the loop: don't unroll loops with calls as this could prevent
+  // inlining.
----------------
NickGuy wrote:
> fhahn wrote:
> > This comment seems out of date?
> Doesn't seem like it is to me. If a function call is found, it bails without enabling runtime unrolling. This check is also performed by many other implementations of this hook.
>  If a function call is found,
That's correct, but you are also bailing out on other cases, like instructions with vector types, right?


================
Comment at: llvm/test/Transforms/PhaseOrdering/AArch64/hoisting-required-for-vectorization.ll:107
   %cmp = icmp ult i32 %1, 20000
-  br i1 %cmp, label %for.body, label %for.cond.cleanup
+  br i1 %cmp, label %for.body, label %for.cond.cleanup, !llvm.loop !0
 
----------------
NickGuy wrote:
> fhahn wrote:
> > why do we need to disable unrolling here?
> We don't //need// to disable it, but this test was ballooning in size when unrolling (as expected), while not testing unrolling itself. Without the knowledge of what it's testing for specifically, I didn't want to change it.
I am not sure that's expected. It doesn't look like the loop in the function has a runtime trip count, unless I am missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97947