[Branch Hinting] Add branch hint handling in RemoveUnusedBrs #7706
Conversation
src/ir/branch-hints.h
Outdated
| if (!likely) { | ||
| // We are writing an empty hint. Do not create an empty annotation if one | ||
| // did not exist. | ||
| if (!func->codeAnnotations.count(expr)) { |
There was a problem hiding this comment.
This could be combined into the previous condition with &&.
| // As |applyAndTo|, but now the condition on |to| the OR of |from1| and |from2|. | ||
| inline void applyOrTo(Expression* from1, | ||
| Expression* from2, | ||
| Expression* to, | ||
| Function* func) { |
There was a problem hiding this comment.
From the name and doc comment, I would have expected this to set to as likely if from1 is likely or from2 is likely. I would not have expected it to reason about branch probabilities at all. Can we move this reasoning up into the callers so we don't have such a misleading method name?
There was a problem hiding this comment.
Moving the logic into the callers would require duplicating the large internal comment - it is not trivial how to handle this, so a helper seems natural?
How about applyCombinedOrConditionTo?
| ExpressionManipulator::nop(br); | ||
| BranchHints::clear(br, getFunction()); |
There was a problem hiding this comment.
What happens if we are left with a branch hint on a nop?
There was a problem hiding this comment.
The binary reader and writer ignore such things, but the text printer is simpler and will print it. This just avoids it being printed out.
| (func $join-br_ifs-no (param $x i32) (param $y i32) | ||
| ;; One is missing a hint, so we clear the hint. | ||
| (block $out | ||
| (@metadata.code.branch_hint "\00") |
There was a problem hiding this comment.
If this hint were a 1, it would make sense to keep, but I don't think we would currently keep it.
There was a problem hiding this comment.
Good point, this really should be handled. Done (and other feedback too).
| (br_if $out | ||
| (local.get $x) | ||
| ) | ||
| (@metadata.code.branch_hint "\00") |
| (@metadata.code.branch_hint "\00") | ||
| (br_if $out | ||
| (local.get $x) | ||
| ) | ||
| (@metadata.code.branch_hint "\01") |
There was a problem hiding this comment.
If $x is unlikely but $y is likely, then $x or $y should still be likely, so it seems best to keep the likely hint in this case.
| (if | ||
| (local.get $x) | ||
| (then | ||
| (br_if $out |
There was a problem hiding this comment.
It's probably worth testing the case when only the br_if has the hint as well.
Co-authored-by: Thomas Lively <[email protected]>
Add some utilities for easily updating/copying/clearing branch hints, then use
those in the pass.
As a drive-by, move
flip()fromwasm-builder.h, which everyone was includingbut only this one cpp file was using, and update it in the new location (this
avoids including the new branch hints header in a central place).
Written manually then verified by fuzzing with #7704