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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
