[BOLT][AArch64] Fix strict usage during ADR Relax #71377
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 |
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.
@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.
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.
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.
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.
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.
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.
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.