-
Notifications
You must be signed in to change notification settings - Fork 683
Dedup delegate blobs in emitter #14564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14564
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Cancelled Job, 1 Unrelated FailureAs of commit 50fb2d9 with merge base 684b5fd ( NEW FAILURE - The following job has failed:
CANCELLED JOB - The following job was cancelled. Please retry:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
Summary: Previously we deduplicated entire 'BackendDelegate' blobs using the preprocessed blob. If two BackendDelegate fields have different id or compile specs, it would be disregarded. This diff: 1. Only deduplicates the preprocessed blob. BackendDelegate retains its own compile specs, etc. 2. Removes the per-method 'delegate_cache', as we have a program-wide delegate cache. 3. Adds a test to confirm we have one delegate segment but two BackendDelegate references pointing at it. Differential Revision: D83162107
2712cac
to
f1663df
Compare
processed_bytes = lowered_module.processed_bytes | ||
hashed = hashlib.sha256(processed_bytes).hexdigest() | ||
delegate_index = self.emitter_state.delegate_cache.get(hashed) | ||
delegate_index = self.program_state.backend_delegate_data_cache.get(hashed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between emitter state and program state? How is backend_delegate_data_cache
different than delegate_cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emitter state is per-method, program state covers all the methods in the program
BackendDelegateInlineData(data=processed_bytes) | ||
) | ||
|
||
backend_delegate = BackendDelegate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure why the previous logic didn't work but the new one does. It seems the logic is the same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous logic refers to the BackendDelegate created for the deduplicated processed blob. We may have a different compile specs, but the same processed blob, in which case the compile specs are lost. You can take a look at the test case as well, and try it on the old code.
Instruction(DelegateCall(delegate_index=delegate_index, args=delegate_args)) | ||
Instruction( | ||
DelegateCall( | ||
delegate_index=len(self.emitter_state.delegates) - 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm why is the delegate index defined as len(self.emitter_state.delegates) - 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new logic creates a separate BackendDelegate for each call_delegate, so it corresponds to the length self.emitter_state.delegates.
|
||
plan = program.execution_plan[0] | ||
# Two delegates that point to the same blob. | ||
self.assertEqual(len(plan.delegates), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it for checking the number of call_delegate instruction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Summary: Previously we deduplicated entire 'BackendDelegate' blobs using the preprocessed blob. If two BackendDelegate fields have different id or compile specs, but the same preprocessed blob, we would take the first one and use it in the execution plan. The id/compile specs of the second would be lost. This diff: 1. Only deduplicates the preprocessed blob. BackendDelegate retains its own compile specs, etc. 2. Removes the per-method 'delegate_cache', as we have a program-wide delegate cache. 3. Adds a test to confirm we have one delegate segment but two BackendDelegate references pointing at it. Reviewed By: JacobSzwejbka Differential Revision: D83162107
f1663df
to
b4738be
Compare
Summary: Previously we deduplicated entire 'BackendDelegate' blobs using the preprocessed blob. If two BackendDelegate fields have different id or compile specs, but the same preprocessed blob, we would take the first one and use it in the execution plan. The id/compile specs of the second would be lost. This diff: 1. Only deduplicates the preprocessed blob. BackendDelegate retains its own compile specs, etc. 2. Removes the per-method 'delegate_cache', as we have a program-wide delegate cache. 3. Adds a test to confirm we have one delegate segment but two BackendDelegate references pointing at it. Reviewed By: JacobSzwejbka Differential Revision: D83162107
b4738be
to
50fb2d9
Compare
@pytorchbot cherry-pick --onto release/1.0 -c critical |
Cherry picking #14564The cherry pick PR is at #14658 and it is recommended to link a critical cherry pick PR with an issue. The following tracker issues are updated: Details for Dev Infra teamRaised by workflow job |
Summary:
Previously we deduplicated entire 'BackendDelegate' blobs using the preprocessed blob. If two BackendDelegate fields have different id or compile specs, it would be disregarded.
This diff:
Differential Revision: D83162107