-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Thanks @piiswrong for updating the code this is related to this PR #10817 The
Should we have an assert: if static_shape:
assert static_alloc, "static_alloc must be `True` if static_shape is `True`" What is the consequence of having one without the other? It would be interesting to have a benchmark as well. |
@@ -674,7 +674,7 @@ OpStatePtr CachedOp::StaticForward( | |||
std::lock_guard<std::mutex> lock(state.mutex); | |||
|
|||
bool match = SetForwardGraph(&state.info, recording, inputs); | |||
match = match && state.recording != recording; | |||
match = match && state.recording == recording; |
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.
Test?
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.
It only affects performance. hard to test
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.
Well this is a logical expression and it seems like it did something that it was not supposed it was to do due to a wrong boolean expression.
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.
cache was invalidated unnecessarily
Can you add extra documentation for these two items please? What do they control exactly and how would I know what value to set?
|
Moved to new PR due to CI failure |
Description
(Brief description on what this PR is about)
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments