Make the project compatible with FetchContent and find_package#200
Make the project compatible with FetchContent and find_package#200kurapov-peter merged 4 commits intointel:mainfrom
Conversation
AndreyPavlenko
force-pushed
the
pkg
branch
5 times, most recently
from
3535da3 to
ae30041
Compare
kurapov-peter
left a comment
kurapov-peter
left a comment
There was a problem hiding this comment.
Great patch, thanks! I've put some minor comments and questions. Otherwise looks good. Could you also add a smoke test for exporting functionality? Like an example that uses cmake's find_package.
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| #ifdef GC_HAS_ONEDNN_DIALECT |
There was a problem hiding this comment.
Can we not include the file in the build instead?
There was a problem hiding this comment.
Yes, but in this case I'm getting the error:
LLVM's build system enforces that all source files are added to a build target
I could suppress the error, but I'm not sure if it's a better solution.
There was a problem hiding this comment.
We can conditionally add it to the global property that forms an appropriate library (be that gcpasses or a separate one, no matter). I had a similar problem in #205
CMakeLists.txt
Outdated
| GcCpuRuntime | ||
| GcGpuPasses | ||
| GcInterface | ||
| GcJitWrapper | ||
| GcPasses | ||
| GcUtilsIR | ||
| MLIRCPURuntimeDialect | ||
| MLIRCPURuntimeTransforms | ||
| MLIRLinalgx | ||
| MLIRMicrokernel |
There was a problem hiding this comment.
So these are to cover the static scenario. I think this should eventually be passed through via a global property to be flexible.
There was a problem hiding this comment.
Do you mean so that each module adds its targets to the global property?
CMakeLists.txt
Outdated
| option(GC_ENABLE_TEST "Build the tests" ON) | ||
| option(GC_ENABLE_TEST_DNNL "Build the dnnl tests" ${GC_ENABLE_DNNL}) | ||
| option(GC_ENABLE_TEST_MLIR "Build the mlir tests" ON) | ||
| option(GC_ENABLE_OPT "Build gc-opt" ON) |
There was a problem hiding this comment.
Can be more generic, i.e. GC_ENABLE_TOOLS.
to simplify integration with thirdparty projects. - Use the Gc prefix in the target names. - Added a separate shared library for oneDNN integration - GcDnnl. - The GcDnnl library and the OneDNNGraphDialect are not built if GC_ENABLE_DNNL=OFF. - Added new build options: GC_ENABLE_TEST(renamed), GC_ENABLE_TEST_DNNL, GC_ENABLE_TEST_MLIR, GC_ENABLE_OPT. - Renamed the GC_USE_GPU option to GC_ENABLE_GPU to be consistent with other options. - Export the OpenMP flags to resolve the "JIT session error: Symbols not found: __kmpc_...". - Minor compile.sh script enhancements.
AndreyPavlenko
force-pushed
the
pkg
branch
11 times, most recently
from
f8dfaac to
0f20408
Compare
|
I would suggest splitting the PR into several PRs which match items in the list, it just very hard to follow when we have all the items mixed. At least I suggest splitting it into two parts: essential and non-essential like renamings. |
leshikus
left a comment
leshikus
left a comment
There was a problem hiding this comment.
LGTM I cannot see any style violations, yet as I've said before big changes like this are hard to follow
Sorry for that. Initially it was much smaller... |
3 participants
to simplify integration with thirdparty projects.