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

Kernel hint design #6790

Merged
merged 4 commits into from
Dec 21, 2017
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions doc/design/kernel_hint_design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
## Problem
In PaddlePaddle's [Design](https://github.com/PaddlePaddle/Paddle/blob/develop/doc/design/switch_kernel.md), one Operator may have multiple kernels. Users may have some personal preference to choose a certain type of kernel for an operator, such as `force_cpu` to use a CPU kernel, `use_cudnn` to choose a CUDNN kernel, we need to provide a way for a user to do this.

In the current design, we use KernelType to describe one kernel.

```cpp
struct KernelType {
Place place_;
DataType data_type_;
LayoutType layout_;
};
```
`place_` `data_type_` and `layout_` can come from the input tensor of the operator, `GetActualKernelType(inputs)` use inputs to infer the proper kernel key that fit the incoming data, user can not config it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Users cannot configure it.

Actually, users can configure input data and influence the actual kernel type.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


The design also provides a virtual method `GetExpectedKernelType` that user can overload and choose the KernelType they want to use.

so, we should send the information user defined in proto to `GetExpectedKernelType` for choosing a kernel.

The problem is, how should we define and send the information for `GetExpectedKernelType` to use?

## Solution

### potential choice
1, Do nothing, let the user add the information they want to operator‘s attribute and get them inside `GetExpectedKernelType`, this can work right. But there is a little problem that users may define many kinds of hints for the same purpose, such as `force_cpu`, `use_cpu`, `CPU` for CPU kernel, and `use_cudnn`, `force_cudnn`, `cudnn_kernel` for use of CUDNN kernel.
Copy link
Collaborator

Choose a reason for hiding this comment

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

1, --> 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can define some global constant for these attribute name.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


2, Pre-define all the needed option and use a single attr key such as `kernel_hint` for the user, this is not so flexible if the user wants to define some more kind of hint.

### final choice
To provide enough flexibility while avoiding confusion definition, we can predefine some options, such as `force_cpu`, `use_cudnn`, `use_mkldnn` for a user to choose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using string to represent force_cpu, use_cudnn, use_mkldnn is not extendable. How to make an operator use CPU forcibly and use MKLDNN?

```cpp
const std::string kNonHint = "";
const std::string kForceCPU = "force_cpu";
const std::string kUseCUDNN = "use_cudnn";
const std::string kUseMKLDNN = "use_mkldnn";

KernelType GetExpectedKernelTyp() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

// "kernel_hint" is a user defined attribute name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think kernel_hint is a formal name. It is confusing for users.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, use force_cpu as key and bool as value.

if (Attr<std::string>("kernel_hint") == kForceCPU) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use Attr<bool>(kForceCPU) ?

Copy link
Member Author

@jacquesqiao jacquesqiao Dec 20, 2017

Choose a reason for hiding this comment

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

good idea, done

return KernelType(CPUPlace, ...)
} else {
...
}
}
```

In Python code

```python
def xx_layer(..., kernel_hint=None):
layer_helper = ...
layer_helper .append_op(
type="xx",
# "kernel_hint" should be the same with the attr name in CPP
attr={"kernel_hint": kernel_hint or ""})
```