[TailDuplication] Add option to enable tail duplication of fallthrough BBs #727
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
…uplication of fallthrough BBs Signed-off-by: Vladimir Radosavljevic <[email protected]>
vladimirradosavljevic
changed the title
vladimirradosavljevic
added a commit
that referenced
this pull request
Oct 29, 2024
…h BBs During TailDuplication pass, fallthrough BBs are not duplicated. This is fine for most cases, but there are cases where we want to duplicate fallthrough BBs as well (e.g. EvmEmulator). This patch adds the `tail-dup-fallthrough-bbs` option to enable tail duplication of fallthrough BBs. PR: #727. Signed-off-by: Vladimir Radosavljevic <[email protected]>
vladimirradosavljevic
force-pushed
the
tail_duplicate_fallthrough_bbs
branch
from
481bb85 to
53efc24
Compare
akiramenai
reviewed
Oct 30, 2024
There was a problem hiding this comment.
I assume it improved numbers on EvmEmulator. Could we provide clearer guidance on when this option is beneficial?
There are cases where we want to duplicate fallthrough BBs as well (e.g. EvmEmulator)
This explanation is vague. Additionally, the test case removes an empty BB (which ideally shouldn’t appear post-optimization), it doesn’t provide guidance - though it works fine as a test.
Could this be upstreamed?
…h BBs During the TailDuplication pass, fallthrough BBs are not duplicated. This is generally acceptable, but in scenarios where a loop contains a large switch case and the latch BB is a fallthrough, duplicating the latch BB can eliminate jump instructions in its predecessors. This patch introduces the `tail-dup-fallthrough-bbs` option to enable the duplication of fallthrough BBs. PR: #727. Signed-off-by: Vladimir Radosavljevic <[email protected]>
vladimirradosavljevic
force-pushed
the
tail_duplicate_fallthrough_bbs
branch
from
53efc24 to
2b28888
Compare
Yes, you can see the numbers here (please use edit button to check the numbers before this).
I have changed the explanation, so PTAL.
To provide guidance through the test case, we would need to create a test which contains a loop with a switch case and a latch BB that is a fallthrough, and I don't think it is worth it, since current test is testing the option we introduced.
I tried to enable this by default, but the numbers are not clear win. This option is to cover an edge case that is not common, so we wouldn't have anything to convince the community that this should be merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.