Optimization: Be less conservative in implementation of unread_after(). #403
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
|
LGTM |
lgritz
force-pushed
the
lg-unread
branch
3 times, most recently
from
b4e997d
to
6485fd1
Compare
|
It looks like this review was forgotten about (github still thinks it can merge it). Is that really the case? |
|
It looks like it just never got committed -- the code still has the original |
|
I'm still sitting on this one, need to return to it. What happened is that I in my pre-commit tests, I saw an image difference that needs to be investigated. The change should not have been able to make any visual changes. |
|
Sheesh, has it been that long? In my mind, I had put this off for a couple weeks. Oops. I'll get back to this. |
lgritz
force-pushed
the
lg-unread
branch
2 times, most recently
from
c3f2278
to
bda2fff
Compare
It was really implemented as if it were "unread AT OR after". It's used
to determine whether it's safe to elide assignments to variables that
aren't subsequently use. But sometimes the assignment is a function
where the variable assigned to is also one of the arguments. For
example,
x = func(x,y)
x is read on that op, but not AFTER. But because of how the unread_after
also discounted elision if used on that op, it was less aggressive than it
could have been in eliminating statements like this.
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.
It was really implemented as if it were "unread AT OR after". It's used to determine whether it's safe to elide assignments to variables that aren't subsequently use. But sometimes the assignment is a function where the variable assigned to is also one of the arguments. For example,
x is read on that op, but not AFTER. But because of how the unread_after also discounted elision if used on that op, it was less aggressive than it could have been in eliminating statements like this.
I'm seeing around a 1/2 - 1% speedup on production frames because of this, with a similar decrease in post-optimized symbols and instructions. Not dramatic, but every little bit counts.