-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MKLDNN] Use MKLDNNRun #16772
[MKLDNN] Use MKLDNNRun #16772
Conversation
LGTM but let's wait for the performance testing reports. |
Do we fix any specific issue with these changes or they're just for code refactoring? |
Not for any particular issue, but a code refactoring to improve code quality. Currently, ndarray view check doesn't cover all mkldnn operators, which may cause trouble in rare case. This is to prevent that happen. |
@TaoLv is this refactor OK? |
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.
Looks good :)
One question: MKLDNNRun is checking and doing reorder for an input, how about multi-inputs Ops like elemwise_add
(MKLDNNSumForward
), better to check all the inputs, right?
@ciyongch Yes. MKLDNNRun has 2 versions. One is for unary ops, and other one is for ops with multiple inputs & outputs. elemwise_add will use the second version, and all inputs are well handled. |
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.
@ZhennanQin I see, I didn't realize that version was already in the master branch.
@ZhennanQin please rebase the code |
Description
MKLDNNRun will handle ndarray view and enhance code quality.
@PatricZhao @TaoLv
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments