Skip to content

Refactor inline section into more general instruction frequencies #15

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

Merged
merged 6 commits into from
Jun 11, 2025

Conversation

ecmziegler
Copy link
Collaborator

This change renames the inline section to instr_freq and allows such frequencies for arbitrary instructions with the given frequencies applying to all following instructions.

It also changes the convoluted logarithmic scheme to a more transparent log2 based scheme at an acceptable loss of resolution.

This implements the changes requested in issue #8.

This change renames the `inlining` section to `instr_freq` and allows such
frequencies for arbitrary instructions with the given frequencies applying
to all following instructions.

It also changes the convoluted logarithmic scheme to a more transparent
log2 based scheme at an acceptable loss of resolution.
Copy link
Contributor

@eqrion eqrion left a comment

Choose a reason for hiding this comment

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

LGTM, just a few suggestions around places that seemed to need some generalization.

| 48| +16| 65536 |
| 56| +24| 1.678e+07|
| 64| +32| >4.295e+09|
| 127| |*always inline* |

If the *byte offset* is 0, the hint applies to all call sites where the function is the **target**. It serves as a shorthand notation unless explicitly overridden. In this case, the call frequency should be a rough estimate of the average call frequency of all potential sites. *Note: This should likely be moved to a dedicated section for clearer separation, e.g. `metadata.function.inline` if such a namespace will be supported by custom annotations.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this part still make sense if we have moved to general instruction frequencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm not too happy about that part and I'd be in favor of dropping it unless tool chains insist on it.

@ecmziegler ecmziegler merged commit 61bad0f into main Jun 11, 2025
@ecmziegler ecmziegler deleted the instr_freq branch June 11, 2025 11:54
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