This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kmclaughlin on Jun 24 2020, 3:42 AM.

Details

Summary

Bfloat16 support added for the following intrinsics:

  • ST1
  • STNT1

Diff Detail

Event Timeline

kmclaughlin created this revision.Jun 24 2020, 3:42 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 24 2020, 3:42 AM
fpetrogalli accepted this revision.Jun 24 2020, 7:25 AM

LGTM, just one nit.

Francesco

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1-bfloat.c
5

Nit: is it worth adding the ASM-NOT: warning check that is used in other tests? Of course, only if it doesn't fail, for in such case we would have to address the problem in a separate patch.

(Same for all the new C tests added in this patch).

This revision is now accepted and ready to land.Jun 24 2020, 7:25 AM
  • Added [HasBF16] predicate to new store pattern in AArch64SVEInstrInfo.td
  • Check hasBF16() is true for bfloat16 types in performST1Combine/performSTNT1Combine
  • Added bfloat16 test to sve-pred-contiguous-ldst-addressing-mode-reg-reg.ll
fpetrogalli requested changes to this revision.Jun 24 2020, 11:50 AM

Thank you for updating the patch with the missing tests. I only have one request for the code involving assertions, and the use of let Predicates = ....

Francesco

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12131–12132

I think there is no need to set up a variable here, you can fold this directly in the assertion. Same for the similar change below.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1597–1602

nit: I think this change is not necessary, but if you really want to do it, you should probably move the 4th column, not the second one.

1601

Doesn't this also need HasSVE?

This revision now requires changes to proceed.Jun 24 2020, 11:50 AM
  • Added HasSVE to Predicates in AArch64SVEInstrInfo.td
  • Removed unnecessary indentation changes in AArch64SVEInstrInfo.td
  • Removed hasBF16 variable from performST1Combine/performSTNT1Combine
kmclaughlin marked 4 inline comments as done.Jun 25 2020, 6:41 AM

Thanks for reviewing this again, @fpetrogalli!

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1-bfloat.c
5

Adding the check doesn't fail, but I will add these checks to the load & store tests in a separate patch

fpetrogalli accepted this revision.Jun 25 2020, 7:46 AM

LGTM, thank you!

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1-bfloat.c
5

Yep - thanks for confirming.

This revision is now accepted and ready to land.Jun 25 2020, 7:46 AM
This revision was automatically updated to reflect the committed changes.