This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Rename internal type identifier(s) for __bf16 to BF16Ty
AbandonedPublic

Authored by codemzs on May 10 2023, 12:56 PM.

Details

Summary

This change updates internal type identifiers for __bf16 from BFloat16Ty to BF16Ty and renames any associated variables and function names accordingly. The rationale for this change comes from the review feedback on https://reviews.llvm.org/D149573, which pointed out the confusing naming issues between __bf16 and the upcoming std::bfloat16_t. This modification only affects LLVM/Clang specific code and does not interfere with target-specific code, such as NeonEmitters, SVE Type, AArch64, X86, etc. It is important to note that BF16Ty will represent the storage-only __bf16 type and bfloat16 format, which will be shared with the upcoming std::bfloat16_t type. However, std::bfloat16_t will have the correct semantics and mangling to be an arithmetic type. The existing names are being updated to enhance clarity and maintainability in the codebase. The change is made in a separate patch, as suggested in the review, to ensure a smooth integration of std::bfloat16_t support.

Diff Detail

Event Timeline

codemzs created this revision.May 10 2023, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 12:56 PM
codemzs requested review of this revision.May 10 2023, 12:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 10 2023, 12:56 PM
codemzs edited the summary of this revision. (Show Details)May 10 2023, 1:10 PM
tahonermann requested changes to this revision.May 10 2023, 4:06 PM

This looks great, thank you for doing this!

Requested changes are just to undo some of the style changes.

clang/include/clang/Basic/Specifiers.h
84–89

Whitespace changes to code that is otherwise not being changed are discouraged due to the impact on tools like git blame. If you want to make such a change, please do so as a separate commit and add the commit to .git-blame-ignore-revs.

clang/lib/AST/ASTContext.cpp
7075–7076

Please keep the same style here (I know clang-format might want to do differently; just ignore it in this case).

clang/lib/AST/ItaniumMangle.cpp
3617–3618

Likewise, please maintain consistent style here.

Thank you for *not* changing the literal! :)

clang/lib/Sema/DeclSpec.cpp
590–591

Please maintain consistent style here.

This revision now requires changes to proceed.May 10 2023, 4:06 PM
codemzs updated this revision to Diff 521163.May 10 2023, 4:54 PM
codemzs marked 4 inline comments as done.
codemzs retitled this revision from [Clang] Rename internal type identifier(s) for `__bf16` to `BF16Ty` to [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty.

PR feedback: Revert style changes.

codemzs added a comment.EditedMay 10 2023, 4:55 PM

@tahonermann Thank you for pointing that out and for reviewing my code. I appreciate your guidance. I was following the LLVM contribution guidelines to use git clang-format, but I understand the importance of maintaining existing code styles that may be altered by git-clang format. I will be more mindful of this in the future. I have reverted the style changes and updated the patch as per your feedback.

I was following the LLVM contribution guidelines to use git clang-format, but I understand the importance of maintaining existing code styles that may be altered by git-clang format.

The guidelines are slightly in conflict in that regard so, yeah, its a judgement call.

I added two more suggested edits targeting some comments.

This looks good to me, but I think we should make sure AMD and ARM folks are aware of the change. @foad, @lenary, any concerns?

clang/include/clang/Basic/TargetInfo.h
650–651
clang/include/clang/Basic/arm_neon_incl.td
240
tahonermann added a subscriber: Restricted Project.May 11 2023, 9:41 AM

Heads up @clang-vendors. This patch changes the names of many internal identifiers for enumerators, class data members, and functions (including virtual functions) that are related to support for the __bf16 type. The changes are roughly to replace "bfloat16" with "bf16" to make the association obvious and to avoid confusion when support for the C++ std::bfloat16_t type lands (and to make room for a potential matching _BFloat16 type in C in the future). There are no changes to ABI or code generation; mangling continues to use the same names even where those have "bfloat16" in them. These changes will likely cause compilation failures in downstream projects that will require (simple) identifier updates to resolve.

codemzs updated this revision to Diff 521353.May 11 2023, 9:45 AM
codemzs marked 2 inline comments as done.
codemzs set the repository for this revision to rG LLVM Github Monorepo.

Update comments as per feedback from @tahonermann

barannikov88 added inline comments.
clang/lib/AST/Type.cpp
2189

Looks like another clang-format quirk.

clang/lib/CodeGen/TargetInfo.cpp
5567–5568

Should the name of the method be updated?

tahonermann accepted this revision.May 11 2023, 9:56 AM

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But please wait a few days for recently subscribed folks to have a chance to comment before landing this.

This revision is now accepted and ready to land.May 11 2023, 9:56 AM

The summary as it is will be hard to read in git log. Please split it into multiple lines 72~80 chars each.
https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

codemzs marked an inline comment as done.May 11 2023, 10:10 AM

The summary as it is will be hard to read in git log. Please split it into multiple lines 72~80 chars each.
https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

I've provided the git log message that I see on my end below. I ensured that it is split into multiple lines, with each line not exceeding the character limit. Please let me know if this isn't inline with the LLVM contribution standards.

I've provided the git log message that I see on my end below. I ensured that it is split into multiple lines, with each line not exceeding the character limit. Please let me know if this isn't inline with the LLVM contribution standards.

Looks great, thanks.
One note: if you are going to land the changes via arcanist, you will need to update the
message in the web form. (arc ignores git commit message and takes it from the web.)

codemzs updated this revision to Diff 521363.May 11 2023, 10:23 AM
codemzs marked an inline comment as done.

Addressing feedback from @barannikov88

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But please wait a few days for recently subscribed folks to have a chance to comment before landing this.

Thank you, @tahonermann, for the review. I will hold off on landing this until next week to allow sufficient time for others who are subscribed to review as well.

skan added a subscriber: skan.May 11 2023, 6:51 PM
stuij accepted this revision.May 12 2023, 2:32 AM

Regarding this particular change, we at Arm are happy with the name change. I saw BFloat16 instead of BF16 as tech debt that we didn't get round to fixing.

I do wonder if we need two bfloat implementations, but for that I'll leave a comment on D149573.

BTW, unfortunately @lenary doesn't work at Arm anymore.

tahonermann requested changes to this revision.May 12 2023, 9:27 AM

I do wonder if we need two bfloat implementations, but for that I'll leave a comment on D149573.

Given the discussions occurring in D149573, let's hold off on landing this for now. It is sounding like we might have direction for repurposing __bf16 for std::bfloat16_t (and a future _BFloat16 C type; with __bf16 retained as an alternate spelling). If we go in that direction, then we would presumably want a change that goes in the opposite direction of this patch; a change that migrates "bf16" names towards "bfloat16". Let's focus on confirming that direction first. I'll mark this as requesting changes for now while we figure this out.

This revision now requires changes to proceed.May 12 2023, 9:27 AM

I do wonder if we need two bfloat implementations, but for that I'll leave a comment on D149573.

Given the discussions occurring in D149573, let's hold off on landing this for now. It is sounding like we might have direction for repurposing __bf16 for std::bfloat16_t (and a future _BFloat16 C type; with __bf16 retained as an alternate spelling). If we go in that direction, then we would presumably want a change that goes in the opposite direction of this patch; a change that migrates "bf16" names towards "bfloat16". Let's focus on confirming that direction first. I'll mark this as requesting changes for now while we figure this out.

Thank you for your guidance, @tahonermann. I agree it's prudent to establish a confirmed direction for reusing __bf16 to implement std::bfloat16_t as an arithmetic type before proceeding. If we decide to take this route, I understand that we'll be looking towards a change that aligns "bf16" names more closely with "bfloat16".

In the meantime, I will revert to my initial change in D149573 which repurposed __bf16 type, and present it for further discussion. This, along with a summary of the RFC discussion, should help us reach a consensus. I welcome any further insights or considerations you might think pertinent to this process.

codemzs abandoned this revision.May 29 2023, 11:48 PM

Closing this as it has been resolved by D150913.