-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix InferStorage for sparse fallback in FullyConnected #11498
Conversation
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.
can we add test that will actually fail without this patch? (e.g rowsparse weight)
@@ -210,17 +210,17 @@ inline static bool BackwardFCStorageType(const nnvm::NodeAttrs& attrs, | |||
CHECK_EQ(in_attrs->size(), 3U); | |||
CHECK_EQ(out_attrs->size(), out_expected); | |||
|
|||
DispatchMode wanted_mode; | |||
#if 0 | |||
bool dispatched = false; | |||
// TODO(zhengda) let's disable MKLDNN for FullyConnected for now. |
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.
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.
ideally we should enable it. i forgot the case that it fails. i don't think MKLDNN FC backward is much faster than our native implementation. so it's not very urgent.
@pengzhao-intel @TaoLv did you check if MKLDNN FC backward works in all cases?
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.
We have enabled this in the #11301 and it passed all unit tests and coverage test in our local.
So, I think MKLDNN FC backward works fine.
@haojin2 could you write a unit test? |
@zheng-da unit test added. |
* fix inferstorage for sparse in FullyConnected * test for the new infer storage
Description
#11448
Checklist
Essentials
Changes
Comments