-
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
[New features]Add function node in phi_kernel for MKLDNN #51073
[New features]Add function node in phi_kernel for MKLDNN #51073
Conversation
你的PR提交成功,感谢你对开源项目的贡献! |
paddle/fluid/framework/new_executor/interpreter/data_transfer.cc
Outdated
Show resolved
Hide resolved
@xinyu-intel, @YangQun1,Please help review the code. |
const AttributeMap &fluid_attrs, | ||
phi::AttributeMap *phi_attrs, | ||
bool has_infer_varkernel_fn) { | ||
// According to "GetKernelTypeForVar" in some ops those have MKLDNN codes, |
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.
// According to "GetKernelTypeForVar" in some ops those have MKLDNN codes, | |
// According to "GetKernelTypeForVar" in some ops executed with oneDNN, |
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.
Thanks for the suggestion.
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.
Thanks. I will change the comment according to your suggestion.
class KernelKey; | ||
class DenseTensor; | ||
/** | ||
* Note: GetKernelTypeForVarContext is currently designed to MKLDNN kernel when |
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.
* Note: GetKernelTypeForVarContext is currently designed to MKLDNN kernel when | |
* Note: GetKernelTypeForVarContext is currently designed for oneDNN kernel when |
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.
Thanks for the suggestion.
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.
Thanks. I will change the comment according to your suggestion.
/** | ||
* Note: GetKernelTypeForVarContext is currently designed to MKLDNN kernel when | ||
* the related memeber function 'GetKernelTypeForVar' is special. It is | ||
* possiable to uesed for other custom hardwares in the future. |
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.
* possiable to uesed for other custom hardwares in the future. | |
* possible to leverage to other vendor libraries in the future. |
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.
Thanks for the suggestion.
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.
Thanks. I will change the comment according to your suggestion.
private: | ||
const KernelKey* kernel_key_; // not owned | ||
// Use AttributeMap in namespace 'phi' to avoid depending 'fuild' | ||
const AttributeMap* attrs_; // not owned |
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.
seems that oneDNN specific stuffs are hided inside the AttributeMap. Do you mind making these as members as this is only used for oneDNN kernel.
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.
Because the attribute name that will be used can be "data_layout" or "data_format" or other for the future vendors, it is brief to using AttributeMap.
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.
Personally, as data_layout/data_format has been defined through the phi API, it is natural to stay in the structure directly. I think you're right that AttributeMap
will make it easier for the future vendors to store more specific stuffs, but at the same time, the content inside the map can be out of controlled by the framework. Both are okay to me.
// Only input require reshaping, weights and | ||
// bias are having shape in NCHW order |
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'm not sure if I understand these correctly. Do you mean "Only input requires changing data_layout"?
Usually, we distinguish shape and layout. Take Tensor{shape={1,3,8,8}, data_format={"NCHW"},stride={192,64,8,1}}
as an example. data_format
means the shape is ordered by NCHW(a.k.a channel_size is 3). stride means it has contiguous data_layout(DataLayout::kNCHW). We can reorder the tensor to kNHWC/KONEDNN but keep the shape unchanged. We can reshape the tensor to {3,8,8}/{24,8}/{1,192} with the layout unchanged.
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 think that shape
data_format
andstride
is a whole, together to represent a tensor。Take Tensor{shape={1,3,8,8}, data_format={"NCHW"},stride={192,64,8,1}}
as an example. If you change the data_format
from NCHW
to NHWC
, the shape
must change from {1,3,8,8}
to {1, 8, 8, 3}
together. Otherwise, the data is wrong when you use.
// bias are having shape in NCHW order | ||
if ((expected_kernel_type.layout() == DataLayout::ONEDNN) && | ||
(tensor.layout() != DataLayout::ONEDNN)) { | ||
auto it = attrs.find("data_layout"); |
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.
Hi, I'm a little confusing about the definition of data_layout
and data_format
in Paddle. In LRNOp::GetKernelTypeForVar, we get the data_format
from Attrs, but here we get the data_layout
instead. Do they have any difference?
BTW, do we also plan to move the GetKernelTypeForVar of LRNOp/Pad2dOp/Pad3dOp to phi? since there are also some MKLDNN specific code.
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.
The Attrs name data_format
and data_layout
have the same meaning. However we can't change them for compatibility.
We also plan to move the GetKernelTypeForVar of LRNOp/Pad2dOp/Pad3dOp to phi soon.
May I ask how the MKLDNN specific code impact the auto generation? Not sure if I understand correctly, currently I didn't find GetKernelTypeForVar related code in the generated api.cc and backward_api.cc files. |
You can find the code automatically generated from |
|
||
const std::string& GetVarName(void) const; | ||
|
||
const DenseTensor& GetTensor(void) const; |
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.
这里会不会出现tensor为null的情况
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.
如果没有给tensor成员赋值,会出现null情况,为确保安全后续加上安全性检查。
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
PR types
New features
PR changes
OPs
Describe
card-67001
执行静态图时,为
inputs
准备数据阶段会根据expected_kernel和每个tensor的定义进行backend、layout、dtype转换。在phi下的kernel借助了GetKernelTypeForVar
和kernel内注册的argsdef进行转换,大部分的GetKernelTypeForVar
函数与动态图统一,但由于MKLDNN的引入,存在部分op的GetKernelTypeForVar
多了一些专门处理MKLDNN kernel的特殊代码,如interpolate_op.cc中:这段逻辑较为特殊并且不符合规范,如果需要自动生成,会导致自动生成脚本变得臃肿。综合考虑后,决定在kernel中增加一个功能和
GetKernelTypeForVar
类似的函数指针,默认缺省,只在GetKernelTypeForVar
有特殊逻辑的MKLDNN kernel中注册,如在OneDnn kernel文件interploate_kernel.cc中增加对应的函数和注册:此接口也能用于未来其他硬件的接入。
When executing a static graph, the stage of preparing data for
inputs
will perform backend, layout, and dtype conversions according to the definition of expected_kernel and each tensor. The kernel under phi is converted with the help ofGetKernelTypeForVar
and the argsdef registered in the kernel. Most of theGetKernelTypeForVar
functions are unified with the dynamic graph. However, due to the introduction of MKLDNN, there are some operators'GetKernelTypeForVar
with special codes, such as in interpolate_op.cc:This piece of logic is special and does not conform to the specification. If it needs to be automatically generated, it will cause the automatic generation script to become bloated. After comprehensive consideration, it is decided to add a function pointer similar to
GetKernelTypeForVar
in the kernel. By default, it is only registered in the MKLDNN kernel with special logic forGetKernelTypeForVar
. For an example, add the function and register the function node in the OneDnn kernel fileinterploate_kernel.cc
: