[CPU][Runtime] Set some CPURuntime funcs visibility to default #294
Conversation
| static thread_local FILOMemoryPool threadMemoryPool_{threadlocalChunkSize}; | ||
|
|
||
| extern "C" void *gcAlignedMalloc(size_t sz) noexcept { | ||
| #if defined _WIN32 || defined __CYGWIN__ |
There was a problem hiding this comment.
Maybe we should move this macro to a shared header in CPURuntime folder, as many cpp files will need it?
There was a problem hiding this comment.
May be move it even higher? I believe it's required not only for CPURuntime. What about renaming gc_version.h to gc.h and putting the common staff there?
There was a problem hiding this comment.
May be move it even higher? I believe it's required not only for CPURuntime.
What about to do this only when there is a real requirement?
renaming gc_version.h to gc.h and putting the common staff there
The name gc_version.h conveys more specific information and may be a better name. The newly added CPURuntime/Utils.h and gc_version.h contains macros with totally different purpose. Currently these is no file which needs to include them both. So putting them together may not be necessary.
There was a problem hiding this comment.
What about to do this only when there is a real requirement?
Currently, except of CPURuntime, we need it at least for lib/gc/ExecutionEngine/OpenCLRuntime/OpenCLRuntimeWrappers.cpp and src/dnnl/dnnl_graph_compiler.cpp
|
Suggest to co-work with @huanghaixin008 . The brgemm library also needs to export the symbols. |
@niuxiaog I will push my changes to this branch |
niuxiaog
changed the title
gcAligned* funcs visibility to default
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if defined _WIN32 || defined __CYGWIN__ |
There was a problem hiding this comment.
Please add the include guard for this header. :)
There was a problem hiding this comment.
Oh Oh Sorry for the mistake.
| #else | ||
| #define OCL_RUNTIME_EXPORT | ||
| #endif // _WIN32 | ||
| #include "gc_utils.h" |
There was a problem hiding this comment.
Need to update the CMakeList.txt to add the directory of newly added "gc_utils.h" to resolve the build error.
There was a problem hiding this comment.
Yes. Thanks for the advice! I will come back to this PR soon.
include/gc_utils.h
Outdated
There was a problem hiding this comment.
I propose moving this file into the gc folder, in order to have all the headers under the single root, and renaming to Utils.h, to be consistent with the naming style of all other headers.
There was a problem hiding this comment.
The folder gc is a mirror of llvm/mlir/include/mlir, which contains real functions. Instead, the gc_version.h and gc_utils.h contain macros, so it may be better to keep them outside.
There was a problem hiding this comment.
gc_utils.h may contain not only macros, for example, logging functions could be added there.
I'm thinking also from the installation point of view. Imagine that we have built a dev package. All these headers are installed to /usr/include and I think it's more convenient to have all the files in a single dir.
| target_include_directories(GcOpenclRuntime PRIVATE | ||
| ${MLIR_INCLUDE_DIRS} | ||
| ${OpenCL_INCLUDE_DIRS} | ||
| ${PROJECT_SOURCE_DIR}/include |
There was a problem hiding this comment.
I think, you can just add GcInterface to target_link_libraries(). It should propagate all the common includes, build options, etc.
There was a problem hiding this comment.
Yes, that's also a reasonable way.
There was a problem hiding this comment.
Do you plan to change this?
There was a problem hiding this comment.
Do you plan to change this?
Which way do you think is better?
There was a problem hiding this comment.
Isn't ${PROJECT_SOURCE_DIR}/include already in include path?
There was a problem hiding this comment.
The ${PROJECT_SOURCE_DIR}/include is not in include path, which causes the including error of gc_utils.h. But now gc_utils.h is moved into /include/gc, I will modify this now. Thanks for the reminder!
|
Please rebase the PR and address the comments, then we can merge it for OV integration. |
To solve issue #293.