This is an archive of the discontinued LLVM Phabricator instance.

[Clang][NFC] Add default `getBFloat16Mangling` impl
AbandonedPublic

Authored by Pierre-vh on Dec 8 2022, 12:22 AM.

Details

Summary

All targets that currently implement __bf16 use the exact same mangled name.
Reduce code duplication by adding that name to the default implementation, like it's done in e.g. getLongDoubleMangling and getFloat128Mangling

Depends on D139398

Diff Detail

Event Timeline

Pierre-vh created this revision.Dec 8 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 12:22 AM
Pierre-vh requested review of this revision.Dec 8 2022, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 12:22 AM

I don't normally handle name mangling, so I can't comment too much here, but I will note that Itanium ABI is planning on using DF16b for std::bfloat16_t: https://github.com/itanium-cxx-abi/cxx-abi/pull/147

I don't normally handle name mangling, so I can't comment too much here, but I will note that Itanium ABI is planning on using DF16b for std::bfloat16_t: https://github.com/itanium-cxx-abi/cxx-abi/pull/147

This is just a NFC so if we're sure targets will soon start to use different mangled names then I can just abandon the commit
I just proposed this change because currently there's no reason not to have a default value for this function

Ping? it's a small NFC and if it's not desired I don't mind abandoning it; I'd just like to remove this diff from the queue.

stuij added a comment.Jan 10 2023, 8:14 AM

I went back and forth on this for a bit. There's a review outstanding to change the mangling name for X86: https://reviews.llvm.org/D136919 to align with the C++23 std::bfloat16_t issue from above. I settled on preferring to be conservative, and not risk at some point accidentally opting an arch into a name that doesn't conform with their ABI.

Pierre-vh abandoned this revision.Jan 10 2023, 11:52 PM

Makes sense to abandon this given that D136919 exists