This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add bfloat16 to outstanding tuple vector intrinsics
ClosedPublic

Authored by c-rhodes on Jun 26 2020, 9:53 AM.

Details

Diff Detail

Event Timeline

c-rhodes created this revision.Jun 26 2020, 9:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
fpetrogalli added inline comments.Jun 26 2020, 2:06 PM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_create2-bfloat.c
10

Nit: these are all new files, so you can safely use clang-format on them, as they will not modify the formatting of pre-existing tests. I know this is not what we have done for most of the files in the ACLE tests, but it makes life so much easier to just run a git clang-format HEAD^ on a patch and kinda forgetting about formatting! Since they are new files, they will not generate conflicts with anything we might have downstream. FWIW, this is my personal preference, so if you prefer to adhere with the manual formatting of other files, I am happy with it.

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_get2-bfloat.c
21

Shouldn't we test also values other then zero? 0,1 for get2, 0,1,2 for get3...

c-rhodes added inline comments.Jun 29 2020, 4:17 AM
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_create2-bfloat.c
10

I agree it's nice to be able to use clang-format and not think about it but in my opinion it's more important to be consistent in formatting across the ACLE tests. I'd prefer a follow-up patch once the ACLE is complete containing the diff produced from running clang-format on clang/test/CodeGen/aarch64-sve-intrinsics / clang/test/CodeGen/aarch64-sve2-intrinsics.

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_get2-bfloat.c
21

Shouldn't we test also values other then zero? 0,1 for get2, 0,1,2 for get3...

We have coverage for all valid indexes in the test suite for these intrinsics, I was about to say it's the same code path regardless of type, which it is on the LLVM side but the intrinsic defs in clang are split so I guess that isn't covered. I'll add extra tests.

c-rhodes updated this revision to Diff 274081.Jun 29 2020, 6:03 AM

Changes:

  • Cover all indexes in get/set ACLE tests
c-rhodes marked an inline comment as done.Jun 29 2020, 6:03 AM
fpetrogalli added inline comments.Jun 29 2020, 6:58 AM
llvm/test/CodeGen/AArch64/sve-intrinsics-create-tuple.ll
1

I think you should use the function attribute trick in this test, not the command line option, like we have done in other tests.

100

nit: remove local_unnamed_addr from all tests you have added.

109

out of curiosity.... why not test the create intrinsic directly?

c-rhodes added inline comments.Jun 29 2020, 8:15 AM
llvm/test/CodeGen/AArch64/sve-intrinsics-create-tuple.ll
100

I copied these from the existing tests, not sure why we used local_unnamed_addr in the first place. I'll create a patch to remove it

109

to test we can copy tuple types between basic blocks

c-rhodes updated this revision to Diff 274138.Jun 29 2020, 8:23 AM

Changes:

  • Use function attribute for +bf16 target feature.
c-rhodes marked an inline comment as done.Jun 29 2020, 8:23 AM
fpetrogalli accepted this revision.Jun 29 2020, 9:17 AM

LGTM, thank you.

This revision is now accepted and ready to land.Jun 29 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.