Skip to content

MKLDNN 1.0.2#2102

Closed
sreekanth-yalachigere wants to merge 3 commits intomicrosoft:masterfrom
sreekanth-yalachigere:PR
Closed

MKLDNN 1.0.2#2102
sreekanth-yalachigere wants to merge 3 commits intomicrosoft:masterfrom
sreekanth-yalachigere:PR

Conversation

@sreekanth-yalachigere
Copy link
Contributor

Upgrading MKL-DNN EP to MKL-DNN 1.0.2

@sreekanth-yalachigere sreekanth-yalachigere requested a review from a team as a code owner October 11, 2019 20:11
@jywu-msft
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 20 pipeline(s).

@faxu faxu mentioned this pull request Oct 11, 2019
@jywu-msft
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 20 pipeline(s).


// temporary switch to toggle between mkldnn-vanilla and mkldnn-subgraph implementation using
// ORT_MKLDNN_SUBGRAPH environment variable
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get rid of this line

mkldnn::engine& cpu_engine,
std::vector<mkldnn::primitive>& net,
mkldnn::memory::format& source_format) {
std::vector<std::unordered_map<int, mkldnn::memory>> &net_args) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: & should be after >>

int subgraph_index = 0;

void Reset() {
SubgraphVariables() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? i thought we got rid of it in #1740

@jywu-msft
Copy link
Member

looks like the windows build fails.

@sreekanth-yalachigere
Copy link
Contributor Author

looks like the windows build fails.

Looking at it. cloned MKL-DNN 1.0.2 is failing. It builds for me on my dev box.

namespace onnxruntime {
namespace mkl_dnn {

#define MKLDNN_EP_LRU_CACHE_DEFAULT_SIZE 500
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be implemented later right? remove for now.

ort_source_desc_ = mkldnn::memory::desc(
{src_dims}, MklDnnType<T>(), ort_source_format_);
source_desc_ = ort_source_desc_;
src_md_.reset(new mkldnn::memory::desc(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can take this opportunity to clean up the smart pointer usage in all these files.
src_md_ = make_shared(...) is preferred and more efficient than reset(new T)
let's clean all this up. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the pointers are unique_ptr and we need make_unique.
I was about to push my changes. I will make these changes and then create a new PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the pointers are unique_ptr and we need make_unique.
I was about to push my changes. I will make these changes and then create a new PR

let's do the cleanup as a separate PR then. priority is to get this one passing the CI's and merged soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. I will close this and create new new PR. As I have merged latest from master repo.

@sreekanth-yalachigere
Copy link
Contributor Author

Creating new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants