-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Conversation
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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
Gentle ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
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.
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.
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.
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.
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()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.