-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Small fixes related to BF16 fusion_gru and fusion_lstm #33295
Conversation
Thanks for your contribution! |
@@ -48,7 +48,8 @@ static int BuildFusion(Graph* graph, const std::string& name_scope, | |||
|
|||
// Create New OpDesc | |||
auto gru_creater = [&](Node* gru, Node* x, Node* weight_x, Node* weight_h, | |||
Node* bias, Node* hidden, Node* fc_bias) { | |||
Node* bias, Node* hidden, Node* fc_bias, | |||
const bool& use_mkldnn) { |
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.
Maybe passing use_mkldnn as value would be better? Same with lstm_creater
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.
You are right, for such small types, it is better to pass by value.
@wozna Hi is this PR ready to review? If yes please ask members to review? because last time we informed Baidu that at least 2 LGTM the PR will be merged. Because I m not sure if this is ready so if it's ready you ask other members to review ok? Thanks |
LGTM. Thank you very much the fix! And thank you for calling and explaining it, the reviewing process get much faster. @juncaipeng Could you please review and merge this PR. If you like, we can call and speed up the reviewing process. |
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.
LGTM
@@ -48,7 +48,8 @@ static int BuildFusion(Graph* graph, const std::string& name_scope, | |||
|
|||
// Create New OpDesc | |||
auto gru_creater = [&](Node* gru, Node* x, Node* weight_x, Node* weight_h, |
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 found the name "gru_creater" a bit confusing. Maybe you could use "gru_creator" ?
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.
Oh yes, it could be a spelling mistake.
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.
LGTM
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.
Please double check the tests as they passed with typo.
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.
LGTM. Thanks a lot
PR types
Bug fixes
PR changes
OPs
Describe
In this PR there are small fixes related to fusion_gru and fusion_lstm:
use_mkldnn
attribute infc_gru_fuse_pass
andfc_lstm_fuse_pass
and tests for that. Thanks to this, I could removemkldnn_placement_pass
in test paddle/fluid/inference/tests/api/analyzer_lexical_analysis_gru_tester.cc that runs twice.fusion_lstm
to the list of bf16 operatorsmkldnn_data_type
attribute tofusion_lstm
, which is necessary to run the model on bfloat16