CANN: Refactor is_matched_graph for better maintainability
#17758
+26
−18
Conversation
This file contains hidden or 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
** Description of the problem ** `is_matched_graph` implementation is a bit confusing because is not so easy to understand the comparison between the current graph and the properties of the cached graphs. The usage of `continue` to skip graphs of different sizes, and a combination of a `break` and a flag to exit the inner loop to simulate a `continue` in the outer loop, make it very difficult to follow the logic. Moreover, the passed arguments and the function name do not match the intended semantics properly. ** Proposed solution ** * Rename the function to `is_matched_graph_in_cache` to better reflect its intention. * Pass only the graph cache list instead of the whole context. * Enclose the graph node traversal check inside a lambda to avoid having to use statements that break the control flow arbitrarily. * Rename variables in a more meaningful way. * Update the corresponding function call.
github-actions
bot
added
ggml
rauletorresc
changed the title
is_matched_graph for better maintainabilityis_matched_graph for better maintainability
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description of the problem
is_matched_graphimplementation is a bit confusing because is not so easy to understand the comparison between the current graph and the properties of the cached graphs. The usage ofcontinueto skip graphs of different sizes, and a combination of abreakand a flag to exit the inner loop to simulate acontinuein the outer loop, make it very difficult to follow the logic. Moreover, the passed arguments and the function name do not match the intended semantics properly.Proposed solution
is_matched_graph_in_cacheto better reflect its intention.