Skip to content

Fix mkldnn related irpasses being added triple times#16225

Closed
luotao1 wants to merge 1 commit intoPaddlePaddle:developfrom
luotao1:mkldnn_placement_pass
Closed

Fix mkldnn related irpasses being added triple times#16225
luotao1 wants to merge 1 commit intoPaddlePaddle:developfrom
luotao1:mkldnn_placement_pass

Conversation

@luotao1
Copy link
Contributor

@luotao1 luotao1 commented Mar 15, 2019

@sfraczek
Copy link

sfraczek commented Mar 15, 2019

Roughly looking, this could work fine unless a user calls AnalysisConfig::EnableMKLDNN() twice. Now the flag use_mkldnn_ has no use in both:

  1. AnalysisConfig - because it is not checked.
  2. PassStrategy - because now I think we assume CpuPassStrategy::EnableMKLDNN() is called only once.

@luotao1
Copy link
Contributor Author

luotao1 commented Mar 15, 2019

Now the flag use_mkldnn_ has no use in both:

flag use_mkldnn_ would be used in

if (config_.use_mkldnn_) {
LOG(INFO) << "MKLDNN is enabled";
argument_.SetMKLDNNEnabledOpTypes(config_.mkldnn_enabled_op_types_);
}

@sfraczek
Copy link

ok

@luotao1 luotao1 requested review from Superjomn and sfraczek March 15, 2019 10:32
@LeoZhao-Intel
Copy link
Contributor

with this change, the issue looks like fixed, but it seems to cause status mismatch between Passes_ and use_mkldnn_ in CpuPassStrategy. use_mkldnn_ in old CpuPassStrategy instance was not copied into new one, while Passes_ copied.

Better to add use_mkldnn_ in CpuPassStrategy copy constructor to avoid this mismatch.

@luotao1
Copy link
Contributor Author

luotao1 commented Mar 18, 2019

use_mkldnn_ in old CpuPassStrategy instance was not copied into new one, while Passes_ copied

use_mkldnn_ will be copied here.

@luotao1 luotao1 requested a review from tensor-tang March 18, 2019 04:05
Copy link
Contributor

@tensor-tang tensor-tang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Need @Superjomn 's double check.

@LeoZhao-Intel
Copy link
Contributor

LeoZhao-Intel commented Mar 18, 2019

use_mkldnn_ in old CpuPassStrategy instance was not copied into new one, while Passes_ copied

use_mkldnn_ will be copied here.
Paddle/paddle/fluid/inference/api/analysis_config.cc

Line 108 in 7458114

CP_MEMBER(use_mkldnn_);

I mean use_mkldnn_ in CpuPassStrategy/PassStrategy, instead of AnalysisConfig,

void AnalysisConfig::EnableMKLDNN() {
#ifdef PADDLE_WITH_MKLDNN
  pass_builder()->EnableMKLDNN();
  use_mkldnn_ = true;
#else
  LOG(ERROR) << "Please compile with MKLDNN first to use MKLDNN";
  use_mkldnn_ = false;
#endif

  Update();
}

There are 2 members named use_mkldnn_ , one is AnaysisConfig, the other in PassStrategy.
In Update(), pass_builder_ will be reset with passes_ copied, but in new pass_builder_ instance, use_mkldnn_ value is still false while passes_ has included mkldnn irpasses already, which is what I mean "mismatch". That's why irpasses were added triple times.

with your change, passes_ will not be added again, but use_mkldnn_ in PassStrategy will be false.

@sfraczek
Copy link

I agree with @LeoZhao-Intel that the use_mkldnn_ left in the PassStrategy now has no clear purpose and something should be done about it.

@luotao1 luotao1 closed this Apr 4, 2019
@luotao1 luotao1 deleted the mkldnn_placement_pass branch April 4, 2019 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mkldnn related irpasses are added triple times when mkldnn is enabled in AnaylsisPredictor

4 participants