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

Wed Mar 3 01:36:33 PST 2021

LemonBoy 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

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

================
Comment at: llvm/test/CodeGen/AArch64/vecreduce-fmin-legalization.ll:68
+; CHECK-NEXT:    ret
+; FP16-LABEL: test_v4f16:
+; FP16:       // %bb.0:
----------------
david-arm wrote:
> It looks like you've added FP16 checks here without the additional RUN line. Similar to my comment in the fmax case perhaps you can just use function attributes instead?
Good catch, I keep forgetting pieces when I `git add`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97840