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
[oneDNN] Improving Graph Rewrite Performance #50932
[oneDNN] Improving Graph Rewrite Performance #50932
Conversation
@penpornk Can you please review this PR ? Thanks! |
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.
Thank you very much for the fix and I'm so sorry for my delay!
bool kernel_registered = false; | ||
|
||
if (kernel_element == registered_kernels_map.end()) { | ||
string kernel = KernelsRegisteredForOp(op_name); |
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.
Nit: Please make the variable name more meaningful for readability.
string kernel = KernelsRegisteredForOp(op_name); | |
string registered_kernels = KernelsRegisteredForOp(op_name); |
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.
Done.
string search_string = label; | ||
search_string += string(";") + string(" T in ["); | ||
search_string += DataType_Name(T) + string("]"); |
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.
Nit: This can be done in one command. (Please help format the source. The line is probably too long.)
string search_string = label; | |
search_string += string(";") + string(" T in ["); | |
search_string += DataType_Name(T) + string("]"); | |
string search_string = label + string("; T in [") + DataType_Name(T) + string("]"); |
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.
Done.
// Map used to store kernel registration status | ||
thread_local static auto* registered_kernels_map = | ||
new absl::flat_hash_map<string, bool>(); |
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.
Does this need to be a global static variable? We try to only have static variable when they are trivially destructible. Can this map be part of some class? Or be inside of IsMklOp()
?
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 it can be in IsMklOp()
. I made that change.
Thank you for reviewing the PR. I have addressed your comments. |
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.
Thank you for the changes!
This PR improves the latency of graph rewrite by avoiding doing kernel registry lookup more than once per kernel. Similar fix was to done before to eager mode, so the refactoring is done to manage and maintain that kernel registry info in one place (mkl_graph_util.h)