[PATCH] D129560: [AArch64] Add target hook for preferPredicateOverEpilogue

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:47
+  enum TailFoldingOpts {
+    TFDisabled = 0x0,
+    TFReductions = 0x01,
----------------
I think you want a couple more cases here:
* `default` for whatever is used when no option is provided
* `all` instead of `enabled`
* `simple` for the styles which currently have no name.  I'm not that bothered about the name used, simple was just the first thing that popped into my head and isn't particularly great.

My thinking is that if the user wants to enable a non-default case they shouldn't need to know what the default is.  Likewise you should be able to be explicit when enabling a subset (i.e. you shouldn't need to guess at what needs to be disable and so can just start by disabling all and then adding the cases they care about.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-tail-folding-option.ll:2
+; RUN: opt < %s -loop-vectorize -sve-tail-folding=disabled -S | FileCheck %s -check-prefix=CHECK-NOTF
+; RUN: opt < %s -loop-vectorize -sve-tail-folding=enabled -S | FileCheck %s -check-prefix=CHECK-TF
+; RUN: opt < %s -loop-vectorize -sve-tail-folding=enabled+noreductions -S | FileCheck %s -check-prefix=CHECK-TF-NORED
----------------
Is it worth having a version of this written as `-sve-tail-folding=disabled+simple+reductions+recurrences`, where `simple` is just whatever you decide to call the set which have no name. I don't think we need all combinations but having one line which enables everything manually will ensure we've no holes or typos.

If you agree with adding a `default` option then we should test that as well because those are the check lines that will change as we support more cases.


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

https://reviews.llvm.org/D129560