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

Mon Apr 19 09:07:25 PDT 2021

NickGuy added a comment.

In D97947#2698661 <https://reviews.llvm.org/D97947#2698661>, @fhahn wrote:

> So does this mean the new behavior will be the default if no CPU is specified? I'm not sure if we are ready for that yet, unless we are confident that the current heuristics work well for out-of-order cores too. (Last time I benchmarked this for out-of-order cores there were a few notable regressions, but it's been a few months since then)

Do you remember where those regressions were, and how important these cases are? I can look into them with these changes to see if they help or hinder.

================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1110
+
+  // Scan the loop: don't unroll loops with calls as this could prevent
+  // inlining.
----------------
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.

================
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

----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97947