This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add bfloat16 support to load intrinsics
ClosedPublic

Authored by kmclaughlin on Jun 22 2020, 6:58 AM.

Details

Summary

Bfloat16 support added for the following intrinsics:

  • LD1
  • LD1RQ
  • LDNT1
  • LDNF1
  • LDFF1

Diff Detail

Event Timeline

kmclaughlin created this revision.Jun 22 2020, 6:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 22 2020, 6:58 AM
fpetrogalli requested changes to this revision.Jun 22 2020, 9:34 AM
fpetrogalli added inline comments.
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ldnf1.c
2–4 ↗(On Diff #272409)

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.

This revision now requires changes to proceed.Jun 22 2020, 9:34 AM
david-arm added inline comments.Jun 23 2020, 12:01 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ldnf1.c
2–4 ↗(On Diff #272409)

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?

sdesmalen added inline comments.Jun 23 2020, 3:18 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ldnf1.c
2–4 ↗(On Diff #272409)

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 #ifdefs, 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)

  • Moved bfloat tests into separate files
  • Added checks to the bfloat test files which test the warnings given when ARM_FEATURE_SVE_BF16 is omitted in the RUN line
fpetrogalli accepted this revision.Jun 23 2020, 11:04 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 23 2020, 11:04 AM
sdesmalen accepted this revision.Jun 24 2020, 2:11 AM

LGTM

clang/include/clang/Basic/arm_sve.td
275

micro nit: doesn't match column indentation of the code around it.

This revision was automatically updated to reflect the committed changes.
sdesmalen added inline comments.Jun 24 2020, 4:02 AM
llvm/test/CodeGen/AArch64/sve-intrinsics-ld1-addressing-mode-reg-imm.ll
217

Sorry I only just spotted this in D82182 , but to match these intrinsics, the llc command needs to be passed +bf16 (and the patterns in the .td file need to be predicated accordingly)