Page MenuHomePhabricator

[llvm][SveEmitter] Emit the bfloat version of `svld1ro`.
ClosedPublic

Authored by fpetrogalli on Jun 5 2020, 1:54 PM.

Details

Summary

The new SVE builtin type __SVBFloat16_t` is used to represent scalable
vectors of bfloat elements.

Diff Detail

Event Timeline

fpetrogalli created this revision.Jun 5 2020, 1:54 PM
Herald added a reviewer: rengolin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Thanks @fpetrogalli, the changes look good to me. Just added some minor comments.

clang/include/clang/Basic/AArch64SVEACLETypes.def
53–54

nit: I'd move this below __SVFloat64_t separated by a newline.

clang/utils/TableGen/SveEmitter.cpp
367

Maybe it's worth adding a

bool isFloatingPoint() const { return Float || BFloat; }

to avoid having to write both isFloat and isBFloat everywhere.

386

It's easier to just assert that ElementBitwidth == 16.

941

It's easier to just assert that ElementBitwidth == 16

stuij added inline comments.Mon, Jun 15, 4:20 PM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c
2

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?

fpetrogalli marked 4 inline comments as done.

Thank you for the review @sdesmalen.

Francesco

sdesmalen added inline comments.Tue, Jun 16, 1:02 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c
2

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

sdesmalen accepted this revision.Tue, Jun 16, 1:08 AM

LGTM

clang/include/clang/Basic/AArch64SVEACLETypes.def
43

nit: odd formatting (of the last \), did you use clang-format?

This revision is now accepted and ready to land.Tue, Jun 16, 1:08 AM
fpetrogalli marked 3 inline comments as done.

Fix formatting issues, remove an unnecessary empty line.

clang/include/clang/Basic/AArch64SVEACLETypes.def
43

Yes, but def files are left untouched. I fixed it.

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c
2

@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 added inline comments.Thu, Jun 18, 4:37 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c
2

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

stuij added inline comments.Thu, Jun 18, 5:11 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1ro-bfloat.c
2

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

stuij accepted this revision.Thu, Jun 18, 9:25 AM

LGTM

This revision was automatically updated to reflect the committed changes.