Skip to content
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

Discussion on integrating MKLDNN to fluid #6815

Closed
QiJune opened this issue Dec 21, 2017 · 2 comments · Fixed by #6882
Closed

Discussion on integrating MKLDNN to fluid #6815

QiJune opened this issue Dec 21, 2017 · 2 comments · Fixed by #6882

Comments

@QiJune
Copy link
Member

QiJune commented Dec 21, 2017

1. Background and consensus

2. concerns on Layout as the key of OpKernelType

The essence about taking layout as the key OpKernelType is to write following codes in which part.

  • We can get proper Layout in GetExpectedKernelType method and convert data in Trans method, and then chose right kernel registered in advance.
  • We can also put these codes inside the Compute method of MKLDNN kernel. The layout will be chosen and transformed inside kernel.

@tensor-tang Could you give some advice on the total effort of these two choice considering integrating MKLDNN to fluid.

3. Some other related question

Some operators like dropout and batch norm will have different computation logic in train/test process. Fluid handles these logic inside the Compute method of operator kernel. Fluid does not take is_test as a key of OpKernelType currently.

@tensor-tang
Copy link
Contributor

tensor-tang commented Dec 21, 2017

Sure. Currently we have two methods to process the layout, as you listed.

We can get proper Layout in GetExpectedKernelType method and convert data in Trans method, and then chose right kernel registered in advance.

For this one,

  1. A reminder, we need to ensure passing ExecutionContext to GetExpectedKernelType, since mkldnn need the setting of this op(eg. conv) and the MKLDNNDeviceContext to confirm the best layout. (This layout depends on platform and settings.)

After we get the best layout, the code of choosing best layout would also be repeated in Compute function anyway to confirm other format like weight and bias.

  1. Trans function also need ExecutionContext to run the conversion, and here we should have:
    a. the paddle:tensor after converted.
    b. mkldnn::memory of both before and after.
    c. the handle(mkldnn::reorder) of conversion.

Then mkldnn will need to execute the conversion inside this Trans function. That is to say, mkldnn will need to create the a.b.c every time(This is another topic #6822), and need to execute immediately. However this part is not we expected, we hope the a.b.c can be hold and execute only once inside Compute.

We can also put these codes inside the Compute method of MKLDNN kernel. The layout will be chosen and transformed inside kernel.

I prefer this one, then we do not need to register many layout in paddle level.
All the concerns would be solved corresponding inside the kernel Compute .

Some other related question

About the is_test question, I should add one addition that mkldnn would suffer the same issue as well.
Not just only some particular op, you can see here.

@tensor-tang
Copy link
Contributor

So, after our discussion, the last and only details difference between the design and our proposal is:

  auto expect_kernel_type = GetExpectedKernelType(actual_kernel_type);
  
  auto trans = g_data_transformation_[{actual_kernel_type, expect_kernel_type}];
  
  kernel.run(trans(inputs));

==>

  auto expect_kernel_type = GetExpectedKernelType(actual_kernel_type, ctx);
  
  auto trans = g_data_transformation_[{actual_kernel_type, expect_kernel_type}];
  
  kernel.run(trans(inputs, ctx));

Right?
And after the trans, the input tensor inside kernel compute should be the converted one, right?

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 a pull request may close this issue.

2 participants