[compiler] Optimize instruction reordering #29882
Merged
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
[ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This was referenced Jun 12, 2024
facebook-github-bot
added
the
React Core Team
|
Comparing: 0f56841...693b0e6 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
mvitousek
reviewed
Jun 13, 2024
compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts
Outdated
Show resolved
Hide resolved
| lastAssignment !== undefined && | ||
| lastAssignment < instr.id && | ||
| range !== undefined && | ||
| range.end === range.start // this LoadLocal is used exactly once |
There was a problem hiding this comment.
I don't think I understand what the accessedRanges conditions are enforcing here, or why it matters that a local is used exactly once. It seems like it would be safe to reorder a load to be anywhere between the last place that the local was written to (which this pass enforces) and the first place that the load's lvalue is read from -- which I would expect is enforced by the existing dependency analysis. I'm sure there's something I'm missing here though!
Also -- if the thing we want to validate is just that the local is used exactly once, maybe rather than building an accessed range for the local, we could just count the number of reads?
There was a problem hiding this comment.
Yeah i was on the fence about checking the accessed range or just counting reads. Without some form of this check i saw some cases where a LoadLocal was used twice, but once was at the wrong block scope level, so it would be invalid to reorder to the first usage. I'll update this to just count reads and only reorder for LoadLocals whose temp is used exactly once.
…assignment" [ghstack-poisoned]
…assignment" [ghstack-poisoned]
…assignment" [ghstack-poisoned]
mofeiZ
approved these changes
Jun 21, 2024
mofeiZ
reviewed
Jun 21, 2024
| range !== undefined && | ||
| range.end === range.start // this LoadLocal is used exactly once | ||
| ) { | ||
| console.log(`reorderable: ${name.value}`); |
There was a problem hiding this comment.
nit: should we remove the console.log?
…assignment" [ghstack-poisoned]
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]
josephsavona
changed the title
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 21, 2024
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. ghstack-source-id: ad3e516fa474235ced8c5d56f4541d2a7c413608 Pull Request resolved: #29882
This was referenced Jul 25, 2024
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.
Stack from ghstack (oldest at bottom):
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain.
This PR includes two changes:
LoadLocalto be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly onceIn #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.
This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.
So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.
I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures.