mkldnn activations.relu by sreekanth-yalachigere · Pull Request #86 · microsoft/onnxruntime

sreekanth-yalachigere · 2018-12-04T03:57:25Z

Activator Relu is implemented using MKLDNN.

core/providers/mkldnn/activation
    activations.cc
    activations.h

Modified CPU Relu class definition to enable MKLDNN Relu class to inherit for fallback option

class Relu final : public OpKernel {
to
class Relu : public OpKernel {

msftclas · 2018-12-04T03:57:36Z

All CLA requirements met.

pranavsharma · 2018-12-04T05:59:44Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

@@ -0,0 +1,238 @@
+/* Copyright(C) 2018 Intel Corporation


Is there a legal reason to use this license? This has to be under MIT license, not Apache.

I believe this should be ok. Will double check.

pranavsharma · 2018-12-04T06:01:29Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+    ReluPrimitive<T>* relulPrimitive = ReluPrimitivePool<T>::Get(pool_params);
+
+    relulPrimitive->Compute(src_data, dst_data);
+  } catch (mkldnn::error& e) {


const mkldnn::error&

pranavsharma · 2018-12-04T06:01:57Z

onnxruntime/core/providers/mkldnn/activation/activations.h

@@ -0,0 +1,32 @@
+/* Copyright(C) 2018 Intel Corporation


same comment as above for the license

I believe this should be ok. Will double check.

weixingzhang · 2018-12-04T06:28:03Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+  mkldnn::engine& cpu_engine_;
+};
+
+// Pool which allows for reuse of MKLDNN Conv2d primitives which are expensive 


It should be not Conv2d.

Changing Conv2d to Relu

pranavsharma

Blocking this for license related changes.

pranavsharma

Unblocking for now. Will double check.

jywu-msft · 2018-12-04T13:24:57Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+  // Used as the key for Pool Primitive Reuse Pool.
+  std::string ToString() const {
+    std::string key;
+    key.reserve(128);


probably don't need 128 bytes for just a src and dst dims.

will change this to 64 bytes

jywu-msft · 2018-12-04T13:27:37Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+	  return;
+  }
+
+  mkldnn::memory::format GetSrcMemoryFormat() const { return context_.src_fmt; }


GetSrcMemoryFormat and GetDstMemoryFormat are not used?
can get rid of the members (src_fmt, dst_fmt) in context as well then.

jywu-msft · 2018-12-04T13:29:43Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+
+ private:
+  struct ReluContext {
+    mkldnn::memory::format src_fmt;


per comment above. remove src_fmt, and dst_fmt if no need for them.

sreekanth-yalachigere · 2018-12-05T23:22:28Z

@pranavsharma committed the following.
Modified the license header

added const at catch

catch (const mkldnn::error& e)

pranavsharma · 2018-12-06T00:30:30Z

I still don't see MIT license mentioned like other files. You should mention // Licensed under the MIT License. at the top of the file.

sreekanth-yalachigere · 2018-12-06T02:40:32Z

@pranavsharma added second line in header

// Licensed under the MIT License.

pranavsharma · 2018-12-06T02:41:14Z

We did some build pipeline changes recently. Can you please rebase your changes? Thanks.

sreekanth-yalachigere · 2018-12-06T03:10:55Z

@pranavsharma rebase done.

snnn · 2018-12-11T19:02:37Z

onnxruntime/core/providers/mkldnn/activation/activations.cc

+
+// Pool which allows for reuse of MKLDNN Relu primitives which are expensive 
+// to instantiate. To address thread safety, the primitives are stored in a map 
+// on thread local storage.


Who is in the thread local storage?

@snnn kernel objects like Relu lives in thread local storage. This is very similar to @weixingzhang's implementation of Conv and Pool kernels.

This reverts commit 6d80253.

* Revert "mkldnn activations.relu (#86)" This reverts commit 6d80253. * Revert "MKLDNN Sum and Batch Normalizaton kernels (#115)" This reverts commit bdd5e58.

* provider_options * use migraphx_provider_option contants * use migraphx_* constants for printing flags * add kDeviceId

mkldnn activations.relu

0c9899b

sreekanth-yalachigere requested a review from a team as a code owner December 4, 2018 03:57

pranavsharma reviewed Dec 4, 2018

View reviewed changes

pranavsharma requested review from a team and jywu-msft December 4, 2018 06:02

weixingzhang reviewed Dec 4, 2018

View reviewed changes

pranavsharma previously requested changes Dec 4, 2018

View reviewed changes

pranavsharma reviewed Dec 4, 2018

View reviewed changes

jywu-msft reviewed Dec 4, 2018

View reviewed changes

sreekanth-yalachigere added 2 commits December 4, 2018 09:22

updates after PR review 1

1365f61

modified license

69fb110

Added MIT License to header

e61878f

Merge remote-tracking branch 'upstream/master'

1f2fd24

pranavsharma approved these changes Dec 6, 2018

View reviewed changes

pranavsharma merged commit 6d80253 into microsoft:master Dec 6, 2018

snnn reviewed Dec 11, 2018

View reviewed changes

snnn mentioned this pull request Dec 11, 2018

Revert "mkldnn Relu/Sum/BatchNormalization kernels" #147

Merged

snnn pushed a commit that referenced this pull request Dec 11, 2018

Revert "mkldnn activations.relu (#86)"

5eb54ff

This reverts commit 6d80253.

AliceSum mentioned this pull request Mar 27, 2022

[Lot of people have this issue] The model can not "really" do inference; Loading issue is solved. #10990

Closed

hui-li-xf mentioned this pull request Jun 10, 2025

[Mobile] null pointer dereference: SIGSEGV #24991

Closed

Conversation

sreekanth-yalachigere commented Dec 4, 2018

Uh oh!

msftclas commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavsharma Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavsharma left a comment

Choose a reason for hiding this comment

Uh oh!

pranavsharma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreekanth-yalachigere commented Dec 5, 2018

Uh oh!

pranavsharma commented Dec 6, 2018

Uh oh!

sreekanth-yalachigere commented Dec 6, 2018

Uh oh!

pranavsharma commented Dec 6, 2018

Uh oh!

sreekanth-yalachigere commented Dec 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreekanth-yalachigere Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

msftclas commented Dec 4, 2018 •

edited

Loading

pranavsharma Dec 4, 2018 •

edited

Loading

sreekanth-yalachigere Dec 11, 2018 •

edited

Loading