ZhennanQin commented on a change in pull request #14277: Enhance PartitionGraph URL: https://github.com/apache/incubator-mxnet/pull/14277#discussion_r266186588
########## File path: src/operator/subgraph/subgraph_property.h ########## @@ -200,7 +197,7 @@ typedef dmlc::ThreadLocalStore<std::unordered_map<std::string, std::unordered_se #define MXNET_REGISTER_SUBGRAPH_PROPERTY(Name, SubgraphPropertyType) \ static DMLC_ATTRIBUTE_UNUSED auto __make_ ## SubgraphPropertyType ## _ ## Name ## __ = \ - SubgraphPropertyRegistry::Get()->__REGISTER_OR_GET__(#Name, &SubgraphPropertyType::Create) + SubgraphPropertyRegistry::Get()->__REGISTER__(#Name, &SubgraphPropertyType::Create) Review comment: I understand your point. As quantized FC is merged into master. I rebased this PR to show what we needed here: https://github.com/apache/incubator-mxnet/blob/f3b5e980581c94c9d36b5a9f02cbb923be9ba7bd/src/operator/subgraph/mkldnn/mkldnn_subgraph_property.cc#L30-L34 In our design, we will register same backend name many times with different subgraph properties. Property will run inthe order they registered. It's possible to register unique subgraph property many times for different backends or even for unique backend. Below registration is possible in future: ``` MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNConvProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNFCProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN_POST_QUANTIZE, SgMKLDNNConvProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN_POST_QUANTIZE, SgMKLDNNFCProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN_POST_QUANTIZE, SgMKLDNNConvPostQuantizeProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN_POST_QUANTIZE, SgMKLDNNFCPostQuantizeProperty); ``` As you seen, `SgMKLDNNConvProperty` is registered twice for different backend names. Also, it's possible to have this in future: ``` MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNConvProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNFCProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNConversionProperty); MXNET_REGISTER_SUBGRAPH_PROPERTY(MKLDNN, SgMKLDNNConvProperty); ``` We need to register `SgMKLDNNConvProperty` twice within `MKLDNN`, and we need to run each property as the order registered. The reason might be, `SgMKLDNNConversionProperty` will do further optimization on fused conv, and this conversion might expose more opportunities for `SgMKLDNNConvProperty`, so we need to run `SgMKLDNNConvProperty` again. As backend implements more and more subgraph pass, it's highly possible to have above requirement. We need to design for it in advance. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
