[PATCH] D97840: [AArch64] Legalize horizontal fmax/fmin reductions on f16 vectors

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/CodeGen/AArch64/vecreduce-fmax-legalization.ll:3
 ; RUN: llc < %s -mtriple=aarch64-none-linux-gnu -mattr=+neon | FileCheck %s --check-prefix=CHECK
+; RUN: llc < %s -mtriple=aarch64-none-linux-gnu -mattr=+neon,+fullfp16 | FileCheck %s --check-prefix=FP16
 
----------------
david-arm wrote:
> LemonBoy wrote:
> > fhahn wrote:
> > > david-arm wrote:
> > > > Hi, instead of having two RUN lines here would it be better to just use function attributes, i.e. for test_v4f16 have a function attribute that adds "fullfp16" support? That way you only need one RUN line and can avoid all the additional FP16 check lines too, since most of them seem to be the same as the first RUN line.
> > > You could also use multiple check prefixes to avoid having repeated check for both RUN lines, if they are equal, e.g. `FileCheck %s --check-prefix=CHECK --check-prefix=NOFP16 ...`,  `FileCheck %s --check-prefix=CHECK --check-prefix=FP16 ...`
> > > 
> > > With that, we should only need separate `FP16`/`NOFP16` check lines for functions where there is a difference.
> > The main idea is to check the expanded reduction is emitted when `fullfp16` is not specified, I guess the `FP16` check can be removed altogether if we don't care about checking the case where the intrinsic can be lowered.
> Hi @fhahn sure that would work too and it's a good point - I was just thinking that fewer RUN lines meant faster test suites that's all.
> I was just thinking that fewer RUN lines meant faster test suites that's all.

That's also a good point. I'm not sure how things will shake out with more tests for `half`; I think we should have fp16 tests both with and without `fullfp16`, as long we have that and can avoid must of the redundant check lines, I am happy whichever way we go :)


================
Comment at: llvm/test/CodeGen/AArch64/vecreduce-fmax-legalization.ll:67
 
+define half @test_v4f16(<4 x half> %a) nounwind {
+; CHECK-LABEL: test_v4f16:
----------------
can you also throw in a test with vectors that are not directly legal, regardless of `fullfp16`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97840