Page MenuHomePhabricator

[clang][BFloat] Add create/set/get/dup intrinsics
ClosedPublic

Authored by miyuki on May 11 2020, 4:36 AM.

Details

Summary

This patch is part of a series that adds support for the Bfloat16 extension of
the Armv8.6-a architecture, as detailed here:

https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/arm-architecture-developments-armv8-6-a

The bfloat type and its properties are specified in the Arm Architecture
Reference Manual:

https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile

The following people contributed to this patch:

  • Luke Cheeseman
  • Momchil Velikov
  • Luke Geeson
  • Ties Stuij
  • Mikhail Maltsev

Diff Detail

Event Timeline

stuij created this revision.May 11 2020, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 4:36 AM
tschuett retitled this revision from [clangd][BFloat] add create/set/get/dup intrinsics to [clang][BFloat] add create/set/get/dup intrinsics.May 11 2020, 11:58 AM
fpetrogalli added inline comments.May 13 2020, 7:35 AM
clang/test/CodeGen/aarch64-bf16-getset-intrinsics.c
120–121

This seems to be the only place where you need to differentiate between check32 and check64, and I am not 100% sure the extra q in the name of the variable is relevant in terms of codegen testing.

Maybe you can just test both aarch32 and aarch64 with the same CHECK prefix?

kadircet removed a subscriber: kadircet.May 13 2020, 9:21 AM

I was an author for part of this patch. Please add all authors as a list of authors to this commit message. Thanks!

As an aside, it would be worth doing this for all the patches in this series

stuij updated this revision to Diff 264588.May 18 2020, 4:46 AM

adhere to patch attribution conventions: change author to Ties, add all the contributors

Can you update the commit message in this differential as well please? Same for the other commits :)

stuij edited the summary of this revision. (Show Details)May 20 2020, 3:45 AM
labrinea added inline comments.
clang/include/clang/Basic/arm_neon.td
1859

v8.6-A ?

stuij marked an inline comment as done.May 20 2020, 9:57 AM
stuij added inline comments.
clang/include/clang/Basic/arm_neon.td
1859

Yes V8.2-A, the feature has been added to the 8.2 architecture, but in a later release.

For example, the section on BFloat in the Arm ARM reads:
ARMv8.2-BF16, Armv8.2 AArch64 BFloat16 Extension

stuij added a comment.May 21 2020, 5:15 AM

Can you update the commit message in this differential as well please? Same for the other commits :)

done

stuij edited the summary of this revision. (Show Details)May 22 2020, 3:13 AM
stuij updated this revision to Diff 265764.May 22 2020, 10:07 AM

moving 'CartesianProductWith' to more apt patch

dmgreen added inline comments.
clang/include/clang/Basic/arm_neon.td
1874

Do you know what InstName = "vmov" does, and is it needed here?

I'm pretty sure it's a vestige of an earlier implementation of the neon emitter.

1881

Does this need let isLaneQ = 1, like the other vdup_laneq's?

clang/include/clang/Basic/arm_neon_incl.td
293 ↗(On Diff #265764)

Is this needed in this patch?

clang/lib/CodeGen/CGBuiltin.cpp
6313

How come these are needed for vduph_lane_bf16 if they were not needed for vduph_lane_f16?

clang/test/CodeGen/aarch64-bf16-getset-intrinsics.c
3

It's best to have auto generated tests if we can, and ideally not rely on running the entire -O2 pipeline. I think a lot of the other tests use clang ... -disable-O0-optnone | opt -S -mem2reg | Filecheck ...

Also this aarch64 tests is running arm codegen too? Not sure if that matters, but I don't immediately see it being done elsewhere.

20–21

A lot of these 32 and 64 bit lines look the same.

labrinea added inline comments.Jun 1 2020, 8:51 AM
clang/include/clang/Basic/arm_neon.td
1868

My local build points here with:
arm_neon.td:1926:3: error: No compatible intrinsic found - looking up intrinsic 'splat_laneq(bfloat16x8_t, int32_t)'

stuij marked an inline comment as done.Jun 2 2020, 9:24 AM
stuij added inline comments.
clang/include/clang/Basic/arm_neon.td
1868

Thanks, yes we need to upstream another patch to match the upstreamed work already done for other types.

miyuki added a subscriber: miyuki.Jun 3 2020, 5:44 AM
miyuki added inline comments.
clang/include/clang/Basic/arm_neon.td
1874

It looks like InstName was removed from NeonEmitter in

commit dee4ab08ba956efd76aa10da46510dcddecceacf
Author: James Molloy <james.molloy@arm.com>
Date:   Tue Jun 17 13:11:27 2014 +0000

    Rewrite ARM NEON intrinsic emission completely.
miyuki commandeered this revision.Jun 3 2020, 6:04 AM
miyuki added a reviewer: stuij.
miyuki updated this revision to Diff 268162.Jun 3 2020, 6:10 AM
miyuki retitled this revision from [clang][BFloat] add create/set/get/dup intrinsics to [clang][BFloat] Add create/set/get/dup intrinsics.
miyuki edited the summary of this revision. (Show Details)

Addressed reviewers' comments.

miyuki marked 5 inline comments as done.Jun 3 2020, 6:12 AM
labrinea accepted this revision.Jun 3 2020, 6:28 AM
This revision is now accepted and ready to land.Jun 3 2020, 6:28 AM
miyuki updated this revision to Diff 268528.Thu, Jun 4, 10:51 AM

Fixed the "RUN:" line in the A32 test, implemented vduph_lane for A32.

This revision was automatically updated to reflect the committed changes.