Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make _FloatN and _BitInt types as substitution candidates in mangling #159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zsrkmyn
Copy link

@zsrkmyn zsrkmyn commented Jan 9, 2023

As discussed in https://reviews.llvm.org/D140359, encodings of _FloatN and _BitInt(N) are relatively long, and thus should be treated as substitution candidates to shorten mangled names.

As discussed in https://reviews.llvm.org/D140359, encodings of _FloatN
and _BitInt(N) are relatively long, and thus should be treated as
substitution candidates to shorten mangled names.
@rjmccall
Copy link
Collaborator

rjmccall commented Jan 9, 2023

Substantively, this seems reasonable to me.

@zsrkmyn
Copy link
Author

zsrkmyn commented Jan 28, 2023

May I know if there's any concern on this? :-)

@zsrkmyn
Copy link
Author

zsrkmyn commented Feb 23, 2023

@rjmccall, can we get this checked in? Or is there anything else I should do? thanks!

@rjmccall
Copy link
Collaborator

rjmccall commented May 9, 2023

Sorry for losing track of this. Three comments:

  1. Given that we've also added manglings for _Float16x and std::bfloat16_t, I think we need to understand the exact set of types we want to allow to serve as substitution candidates.
  2. These extensions have been around for awhile, so I'd like to understand what existing practice is here and make sure that we have agreement between vendors implementing it.
  3. It'd be nice if we had a more precise way of specifying this list, maybe by splitting up the grammar a bit so that the substitutable builtin types were a different production from the unsubstitutable ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants