stuij added inline comments.
================ Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c:1 +// RUN: %clang_cc1 -D__ARM_FEATURE_SVE_MATMUL_FP64 -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE -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_MATMUL_FP64 -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE -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 ---------------- sdesmalen wrote: > fpetrogalli wrote: > > sdesmalen wrote: > > > stuij wrote: > > > > There should be no dependency on `-fallow-half-arguments-and-returns`. > > > > For bfloat we should use `-mfloat-abi hard`. Does this work for > > > > `-mfloat-abi softfp`? > > > `-fallow-half-arguments-and-returns` isn't strictly needed for this test, > > > we just use the same RUN line for all the ACLE tests and we needed this > > > for `__fp16` in some of the tests. > > > > > > I don't believe that `-mfloat-abi softfp` is supported for AArch64. > > @stuij - the following lines work, one with `softfp` and one with `hard`: > > > > ``` > > // RUN: %clang_cc1 -D__ARM_FEATURE_SVE_MATMUL_FP64 > > -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE > > -DSVE_OVERLOADED_FORMS -triple aarch64-none-linux-gnu -target-feature +sve > > -target\ > > -feature +bf16 -mfloat-abi softfp -fallow-half-arguments-and-returns -S -O1 > > -Werror -Wall -emit-llvm -o - %s | FileCheck %s > > // RUN: %clang_cc1 -D__ARM_FEATURE_SVE_MATMUL_FP64 > > -D__ARM_FEATURE_BF16_SCALAR_ARITHMETIC -D__ARM_FEATURE_SVE > > -DSVE_OVERLOADED_FORMS -triple aarch64-none-linux-gnu -target-feature +sve > > -target\ > > -feature +bf16 -mfloat-abi hard -fallow-half-arguments-and-returns -S -O1 > > -Werror -Wall -emit-llvm -o - %s | FileCheck %s > > ``` > > > > @sdesmalen I am not an experer here, but there is a test which targets > > aarch64 that uses `softfp` (see > > `clang/test/CodeGen/arm-bf16-params-returns.c`). The following line in that > > test clearly targets `aarch64`: > > > > ``` > > // RUN: %clang_cc1 -triple aarch64-arm-none-eabi -target-abi aapcs > > -mfloat-abi softfp -target-feature +bf16 -target-feature +neon -emit-llvm > > -O2 -o - %s | opt -S -mem2reg -sroa | FileCheck %s \ > > --check-prefix=CHECK64-SOFTFP > > ``` > > > > @both - should I update the test with the two extra RUN lines mentioned up > > in the message? > > @sdesmalen I am not an experer here, but there is a test which targets > > aarch64 that uses softfp (see > > clang/test/CodeGen/arm-bf16-params-returns.c). The following line in that > > test clearly targets aarch64: > `clang/test/CodeGen/arm-bf16-params-returns.c` also shows that setting > `softfp` has no effect for AArch64. > > > @both - should I update the test with the two extra RUN lines mentioned up > > in the message? > No, I think the extra RUN lines aren't necessary. @fpetrogalli: @sdesmalen is totally right. Softfp doesn't make sense on AArch64 as fp isn't optional. I think the original intent of that AArch64 line in `arm-bf16-params-returns.c` was to make sure AArch64 indeed doesn't change, but then the option should of course never be passed in the first place. I guess this is a bit of over-defensive coding against the way clang isn't stellar at argument passing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81304/new/ https://reviews.llvm.org/D81304 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits