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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

I was under the impression that without a -mcpu it defaulted to cortex-a53 schedule. It looks like it's no-schedule though, which still counts as an in-order core as it has no MicroOpBufferSize. Can we check if ST->getSchedModel().ProcID != 0, which will be the "NoSchedModel".



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1128
+
+  // Force runtime unrolling for in-order models
+  // If mcpu is omitted, then an in-order model will be used, meaning this
----------------
"Force" doesn't sound like the right wording to me. It is enabling runtime/partial unrolling, which the loop unroller may use or not.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1131
+  // effects the "default" compilation configuration for Aarch64.
+  // TODO This might be beneficial for out-of-order models too
+  if (!ST->getSchedModel().isOutOfOrder()) {
----------------
Out of order cores will already enable runtime unrolling based on the MicroOpBufferSize, it will just have a reduced threshold. I don't think the TODO is useful to add.


================
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:
> 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?
It can still be partially unrolled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97947