Skip to content

[BOLT][AArch64] Fix strict usage during ADR Relax #71377

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 1 commit into from
Nov 10, 2023
Merged

Conversation

yota9
Copy link
Member

@yota9 yota9 commented Nov 6, 2023

Currently strict mode is used to expand number of optimized functions,
not to shrink it. Revert the option usage in the pass, so passing strict
option would relax adr instruction even if there are no nops around it.
Also add check for nop after adr instruction.

@yota9 yota9 added the BOLT label Nov 6, 2023
Currently strict mode is used to expand number of optimized functions,
not to shrink it. Revert the option usage in the pass, so passing strict
option would relax adr instruction even if there are no nops around it.
Also add check for nop after adr instruction.
Copy link

github-actions bot commented Nov 6, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@yota9
Copy link
Member Author

yota9 commented Nov 9, 2023

Gentle ping

Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

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

LGTM

@yota9 yota9 merged commit abec50c into llvm:main Nov 10, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Currently strict mode is used to expand number of optimized functions,
not to shrink it. Revert the option usage in the pass, so passing strict
option would relax adr instruction even if there are no nops around it.
Also add check for nop after adr instruction.
yota9 added a commit to yota9/llvm-project that referenced this pull request Aug 5, 2024
Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by llvm#71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.
yota9 added a commit to yota9/llvm-project that referenced this pull request Aug 5, 2024
Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by llvm#71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.
yota9 added a commit to yota9/llvm-project that referenced this pull request Aug 6, 2024
Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by llvm#71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.
yota9 added a commit to yota9/llvm-project that referenced this pull request Aug 7, 2024
Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by llvm#71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.
yota9 added a commit to yota9/llvm-project that referenced this pull request Aug 7, 2024
Follow the logic of https://reviews.llvm.org/D143887 patch (fixed later
by llvm#71377) we don't want to remove nops in non-simple function just in
case there is undetected jump table to minimize chances to break offsets
in it.
} else if (opts::StrictMode && !BF.isSimple()) {
} else if (std::next(It) != BB.end() && BC.MIB->isNoop(*std::next(It))) {
BB.eraseInstruction(std::next(It));
} else if (!opts::StrictMode && !BF.isSimple()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@yota9, I don't think it's safe to allow code size expansion for non-simple functions in strict mode. Do you remember why this change was needed?

Copy link
Member Author

@yota9 yota9 Dec 11, 2024

Choose a reason for hiding this comment

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

Hello @maksfb . I don't really like strict option usage here. But before this patch (not added by me) it was the opposite, in strict mode there was an error. But strict mode is disabled by default and by default we want the error to be raised in this case. Also strict mode "expands" number of functions bolt can process as we're more "trust" the binary as "well-formed source". So although I fill like there might be separate option used here it was strict used before and I've decided only to fix the condition here I was not agreed with.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The original check was added by @treapster in https://reviews.llvm.org/D143887, and I believe the intent was to put more strict requirements on the binary (?). Anyway, if there are no objections, I'm going to remove the StrictMode check here and always issue an error if we can't update non-simple functions without changing instruction offsets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really mind. Ideally here should be option with a list of functions that are allowed to be skipped with this error. But I think someone who'll need this can implement this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is very confusing because it has relaxed behavior with strict option and strict behavior without it. I think the only way to make it make sense is to fail regardless of --strict mode as @maksfb suggested. But you have to evaluate whether it's worth making bolt always fail on binaries with non-simple functions without nops. If there's no jump table, it is fine so a failure may be false positive.

Copy link
Member Author

@yota9 yota9 Dec 12, 2024

Choose a reason for hiding this comment

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

Although the name of the option might suggests to be more "strict" to the binary input, the options description says the opposite "trust the input to be from a well-formed source". So the option expands the number of binary functions we're processing, rather the shrinks it. Although it doesn't really matter, I've only fixed the option usage here, but the option choice here was not the best from the beginning.
With previous logic there was some interferience with strict in another place (I don't remember) that followed the logic right, this was the reason I had to fix it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm the one to blame for the option name. The idea is that the input binary is built with strict requirements allowing BOLT to optimize more aggressively. The way I see it at the moment, we may get rid of the option in the future. It certainly doesn't make much sense on ARM anyway where we cannot rely on relocations to reconstruct control flow.

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

Successfully merging this pull request may close these issues.

4 participants