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

Add mangling for fixed point types proposed in N1169. #161

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

PiJoules
Copy link

No description provided.

@PiJoules
Copy link
Author

Not sure how to "reference" issues with PRs, but this is for issue #56

abi.html Outdated Show resolved Hide resolved
@PiJoules PiJoules force-pushed the fixed-point-type-mangling branch from 81e56e4 to 0ffd83d Compare May 9, 2023 21:56
Copy link
Collaborator

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me; just a couple minor editorial requests.

We have an active PR in #159 to make certain extended types substitutable. Should these be substitution candidates if the same type is used in multiple places?

abi.html Outdated Show resolved Hide resolved
abi.html Outdated Show resolved Hide resolved
@PiJoules PiJoules force-pushed the fixed-point-type-mangling branch from 0ffd83d to c204e5f Compare May 12, 2023 01:18
@PiJoules
Copy link
Author

We have an active PR in #159 to make certain extended types substitutable. Should these be substitution candidates if the same type is used in multiple places?

I asked around and we unfortunately don't know how common it would be to have mangled signatures with fixed-point types in them in places we might intend to use them. Sorry for the incomplete answer. Although given that the largest mangling for a fixed-point type is 5 characters and substitutions should be less than that, we still think having a sub should provide savings.

@rjmccall
Copy link
Collaborator

Okay. The abstract guideline here is that adding substitutions does have a cost: in longer symbols, it can actually increase overall symbol size by filling the substitution table with a lot of extra types, making later substitutions larger. I would say the line is around 4 bytes — a 3-byte mangling is probably not worth substituting, but a 5-byte mangling probably is.

Applying that to this would make only the saturating types candidates. But treating different fixed-point types differently might be annoying to implement, and it's probably not a big deal either way.

Given that, do you have a strong opinion?

@PiJoules
Copy link
Author

Okay. The abstract guideline here is that adding substitutions does have a cost: in longer symbols, it can actually increase overall symbol size by filling the substitution table with a lot of extra types, making later substitutions larger. I would say the line is around 4 bytes — a 3-byte mangling is probably not worth substituting, but a 5-byte mangling probably is.

Applying that to this would make only the saturating types candidates. But treating different fixed-point types differently might be annoying to implement, and it's probably not a big deal either way.

Given that, do you have a strong opinion?

Yeah in that case I'd probably want to aim for consistency and would lean towards not making any of them candidates.

@PiJoules
Copy link
Author

ping for any more comments on this

@zygoloid
Copy link
Contributor

Looks good to me.

Copy link
Author

@PiJoules PiJoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@PiJoules PiJoules closed this Sep 22, 2023
@PiJoules PiJoules reopened this Sep 22, 2023
@PiJoules PiJoules requested a review from rjmccall September 22, 2023 20:58
@rjmccall
Copy link
Collaborator

Alright, I think this has been open for long enough.

@rjmccall rjmccall merged commit fbb567b into itanium-cxx-abi:main Sep 26, 2023
@PiJoules PiJoules deleted the fixed-point-type-mangling branch May 15, 2024 23:46
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.

3 participants