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

[David Sherwood via Phabricator via llvm-commits]

david-arm 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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82298