Page MenuHomePhabricator

[clang][BFloat] add NEON emitter for bfloat
ClosedPublic

Authored by stuij on May 11 2020, 4:30 AM.

Details

Summary

This patch adds the bfloat16_t struct typedefs (e.g. bfloat16x8x2_t) to
arm_neon.h

This patch is part of a series implementing 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
  • Simon Tatham
  • Ties Stuij

Diff Detail

Event Timeline

stuij created this revision.May 11 2020, 4:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 4:30 AM
tschuett retitled this revision from [clangd][BFloat] add NEON emitter for bfloat to [clang][BFloat] add NEON emitter for bfloat.May 11 2020, 11:57 AM

Hi @stuij ,

thank you for working on this.

Would it make sense to add a test that includes the header file you have created?

Regards,

Francesco

clang/include/clang/Basic/arm_neon_incl.td
293

What is this for?

clang/utils/TableGen/NeonEmitter.cpp
626

Remove me.

2195

Is this related to the changes for bfloat? Or is it a just a refactoring that it is nice to have? If the latter, please consider submitting it as a separate patch. If both refactoring and BF16 related, at the moment it is not possible to see clearly which changes are BF16 specific, so please do submit the refactoring first.

2511

Missing \n

SjoerdMeijer added inline comments.
clang/include/clang/Basic/arm_bf16.td
2

typo: fp16 - > bf16?
Here, and a few more places, or is it intentional? If so, I guess that can be a bit confusing?

clang/utils/TableGen/NeonEmitter.cpp
2351

I am a bit confused here, we already have a runFP16, I am missing something?

2356

I can't remember the outcome, but I had a discussion with @sdesmalen about this license, if this should be the new or old copyright notice. I believe, but am not certain, that this should be the new one.

stuij updated this revision to Diff 264587.May 18 2020, 4:44 AM

adhere to patch attribution conventions

kadircet removed a subscriber: kadircet.May 18 2020, 6:01 AM
stuij edited the summary of this revision. (Show Details)May 20 2020, 3:46 AM
stuij marked 6 inline comments as done.May 21 2020, 9:01 AM
stuij added inline comments.
clang/include/clang/Basic/arm_bf16.td
2

Thanks for that. I went through the patch and I only found the mistake in this file.

clang/include/clang/Basic/arm_neon_incl.td
293

Thanks, that is actually part of another patch. I will remove.

clang/utils/TableGen/NeonEmitter.cpp
2195

Yes, related to bfloat. We're emitting that code twice now.

I can make a new patch, but I'm not sure if the effort justifies the in my mind small amount of gain in clarity. It's basically just pasting the removed part on the left into this function. If you disagree, tell me, and I will create the extra patch.

2351

The fairly similar runBF16 fn was added. The way git interprets this is probably a bit confusing. There's still just one runFP16 function.

2356

I'll change it, and will ping @sdesmalen to be sure.

fpetrogalli added inline comments.May 21 2020, 9:16 AM
clang/utils/TableGen/NeonEmitter.cpp
2195

Thank you. I didn't notice you were invoking this twice. Make sense to have it in a separate function, in this patch.

stuij updated this revision to Diff 265524.May 21 2020, 9:41 AM

addressed review comments, most of all changed license header on the generated bfloat file

stuij marked 2 inline comments as done.May 21 2020, 9:49 AM
stuij added inline comments.
clang/utils/TableGen/NeonEmitter.cpp
2356

Just to be clear: I've changed the BF16 text, not the FP16 text, which should stay the same.

stuij edited the summary of this revision. (Show Details)May 22 2020, 3:12 AM
stuij updated this revision to Diff 265988.May 25 2020, 2:46 AM

replanted arg passing test from other patch

stuij updated this revision to Diff 266085.May 25 2020, 4:11 PM

add bfloat header test

fpetrogalli accepted this revision.May 27 2020, 8:40 AM

LGTM! Might be worth waiting an extra day or two before submitting to make sure the people who provided extra feedback are happy.

This revision is now accepted and ready to land.May 27 2020, 8:40 AM

No objections.
Some nits inlined, which you can ignore if you think they are not correct.

clang/include/clang/Basic/arm_neon_incl.td
218

nit: perhaps bfloat16?

240

nit: perhaps BFloat16?

clang/test/Preprocessor/aarch64-target-features.c
378

nit: BFloat16

clang/test/Preprocessor/arm-target-features.c
855

same

clang/utils/TableGen/NeonEmitter.cpp
744

same?

stuij updated this revision to Diff 268616.Jun 4 2020, 4:13 PM
stuij marked 7 inline comments as done.

addressed remaining nits

This revision was automatically updated to reflect the committed changes.