[PATCH] D82298: [AArch64][SVE] Add bfloat16 support to load intrinsics

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ldnf1.c:2-4
+// RUN: %clang_cc1 -D__ARM_FEATURE_SVE -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE_BF16 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -fallow-half-arguments-and-returns -S -O1 -Werror -Wall -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -D__ARM_FEATURE_SVE -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE_BF16 -DSVE_OVERLOADED_FORMS -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -fallow-half-arguments-and-returns -S -O1 -Werror -Wall -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -D__ARM_FEATURE_SVE -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE_BF16 -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +bf16 -fallow-half-arguments-and-returns -S -O1 -Werror -Wall -o - %s >/dev/null 2>%t
----------------
david-arm wrote:
> fpetrogalli wrote:
> > With @sdesmalen  we where thinking that maybe it is better to duplicate the run lines to have the BF16 intrinsics tested separately:
> > 
> > ```
> >  RUN: %clang_cc1 -D__ARM_FEATURE_SVE  ... -target-feature +sve ...
> >  RUN: %clang_cc1 _DENABLE_BF16_TEST -D__ARM_FEATURE_SVE -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE_BF16 ... -target-feature +sve -target-feature +bf16 ... 
> > ```
> > 
> > and wrap the BF16 tests in `#ifdef ENABLE_BF16_TEST ... #endif`.
> > 
> > this will make sure that the non BF16 tests will be erroneously associated to the BF16 flags.
> > 
> > Please apply these to all the run lines involving BF16 modified in this patch.
> > 
> Is that definite? I mean there is a difference between "we were thinking" and "this is how we are going to do things in future". :) Just to avoid unnecessary code changes that's all. I presume existing tests already written in the same way (committed in last week or so) would be changed too?
The other bfloat test are currently in a separate file (suffixed `-bfloat.c`). @fpetrogalli and I indeed discussed we could do this all in the same file using `#ifdef`s, but for now I'd actually prefer we stick with the approach we have gone down (specific test file for bfloat) until we've changed this for existing tests (in a separate patch).

So for now just move these tests to a separate file and please also add RUN lines like we've done for the SVE2 tests to check that we get diagnostics if `+sve` is passed (without `+bf16`).
(This actually hasn't been done yet for some of the newly introduced bfloat tests, so we'll need to fix that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82298