Ryanunderhill/mkldnn dll - #2114
Conversation
|
note this PR: #2102 |
…o ryanunderhill/mkldnn_dll
|
I think we should build protobuf and onnx as shared library, it will ease the effort of sharing them between DLLs. |
| # list(APPEND onnxruntime_EXTERNAL_LIBRARIES mkldnn) | ||
| # list(APPEND onnxruntime_EXTERNAL_DEPENDENCIES project_mkldnn) | ||
| # link_directories(${MKLDNN_LIB_DIR}) | ||
| #endif() |
There was a problem hiding this comment.
Do you plan on keeping it?
|
You know it is not possible to export STL components. In reply to: 545022067 [](ancestors = 545022067) |
| const int default_max_vlog_level_; | ||
| bool owns_default_logger_; | ||
|
|
||
| public: // TODO: Made public only for the provider bridge. Should be changed to a method to get this instead |
There was a problem hiding this comment.
hould be changed to a method to get this instea [](start = 62, length = 47)
Any reason not to do it in this PR?
| template <> | ||
| MLDataType DataTypeImpl::GetType<bool>() { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Let's remove TABs everywhere.
| } | ||
|
|
||
| // Override default new/delete so that we match the host's allocator | ||
| void* operator new(size_t n) { return g_host->HeapAllocate(n); } |
There was a problem hiding this comment.
You need to add array versions of these and I would add nothrow overloads too so we do not scratch our heads when debugging. Add noexcept to all of delete as well per C++11.
| } | ||
|
|
||
| struct Provider { | ||
| virtual std::shared_ptr<IExecutionProviderFactory> CreateExecutionProviderFactory(int device_id) = 0; |
There was a problem hiding this comment.
std::shared_ptr [](start = 10, length = 15)
Can we return this as unique_ptr and whoever needs can convert that unique_ptr to shared?
| // a specific implementation of a virtual class member. Trying to get a pointer to member of a virtual function will return a thunk that | ||
| // calls the virtual function (which will lead to infinite recursion in the bridge). There is no known way to get the non virtual member | ||
| // function pointer implementation in this case. | ||
| struct ProviderHost { |
There was a problem hiding this comment.
ProviderHost [](start = 7, length = 12)
This struct needs a virtual destructor
| onnx::TensorProto* (*google_protobuf_Arena_CreateMaybeMessage_onnx_TensorProto)(google::protobuf::Arena*); | ||
|
|
||
| // Special functions in bridge_special.h that route through these | ||
| virtual void onnx_AttributeProto_constructor(void* _this) = 0; |
There was a problem hiding this comment.
_this [](start = 53, length = 5)
All leading underscores are not per standard. Someone actually opened an issue on GH
| // calls the virtual function (which will lead to infinite recursion in the bridge). There is no known way to get the non virtual member | ||
| // function pointer implementation in this case. | ||
| struct ProviderHost { | ||
| virtual logging::Logger* LoggingManager_GetDefaultLogger() = 0; |
There was a problem hiding this comment.
_G [](start = 41, length = 2)
Underscore followed by a capital character are all reserved by the implementation no matter where occurs.
|
|
||
| inline int | ||
| ToIntSize(size_t size) { | ||
| return static_cast<int>(size); |
There was a problem hiding this comment.
return static_cast(size); [](start = 2, length = 30)
The value of this function might be greater if in DEBUG builds it could check if the the value of size does not exceed std::numeric_limits::max().
| @@ -0,0 +1,11 @@ | |||
| void onnx_AttributeProto_constructor(void* _this); | |||
There was a problem hiding this comment.
onnx_AttributeProto [](start = 4, length = 20)
Header?
| @@ -0,0 +1,84 @@ | |||
| #include <string> | |||
| #include "bridge_special.h" | |||
| @@ -0,0 +1,18 @@ | |||
| #include <string> | |||
|
|
||
| std::shared_ptr<IExecutionProviderFactory> CreateExecutionProviderFactory_Mkldnn(int device_id) { | ||
| void* handle; | ||
| Env::Default().LoadDynamicLibrary("C:\\code\\github\\onnxrt\\build\\windows\\debug\\debug\\onnxruntime_providers_mkldnn.dll", &handle); |
There was a problem hiding this comment.
"C:\code\github\onnxrt\build\windows\de [](start = 36, length = 45)
Needs to be addressed.
## Describe your changes ## Checklist before requesting a review - [ ] Add unit tests for this change. - [ ] Make sure all tests can pass. - [ ] Update documents if necessary. - [ ] Lint and apply fixes to your code by running `lintrunner -a` - [ ] Is this a user-facing change? If yes, give a description of this change to be included in the release notes. - [ ] Is this PR including examples changes? If yes, please remember to update [example documentation](https://github.com/microsoft/Olive/blob/main/docs/source/examples.md) in a follow-up PR. ## (Optional) Issue link
4 participants
Description: Initial test layer to make mkldnn provider build as a shared library
NOTE: This is still a test, talk to me if you want to seriously review it