-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MKLDNN] NDArray reorder in C API and deconv #16265
Conversation
…to fix/c-api-ndarray
…to fix/c-api-ndarray
@rondogency @matteosal @pengzhao-intel Please review. Thanks. |
I can't actually review, I'm absolutely not familiar with the libraries' internals. But I can confirm that the related issues are fixed by these changes |
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! FYI the deconv fix has been verified on our end, so I think this PR is good to go
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 fix.
The reorder in c_api doesn't synchronized with engine, thus there's a risk that there's some computing working on this NDArray, resulting in unexpected result. |
@ZhennanQin Thank you for reminding. I would expect the C API users can understand they need call @szha any thoughts? Seems we don't have any document for C API? |
Another potential problem is, C API will be called in main thread, which doesn't have computing resource. All computing resource should be managed by engine. Ideally, we'd better to push this operation to engine, and wait to get the reordered result. |
@TaoLv @ZhennanQin do we need another PR to enhance? |
@ZhennanQin Would you mind contributing another PR towards your suggestion? Thanks. |
* layout conversion in MXNDArrayGetData * deconv: layout conversion for grad * fix lint complain
* layout conversion in MXNDArrayGetData * deconv: layout conversion for grad * fix lint complain
Description
Should fix #16143 .
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments