Skip to content

Conversation

@dchigarev
Copy link
Contributor

I don't know why, but LLVM_DEFINITIONS variable (that is populated by find_package(llvm...)) contains all compile definitions that were added prior calling find_package(llvm...) when I build IMEX with OpenVino.

In particular, LLVM_DEFINITIONS contains -DOV_BUILD_POSTFIX=... and -DOV_BUILD_PATH=... variables that were already defined by OV which causes the IMEX build to fail due to double definition (because they were already added as add_definitions() on OV side and now we're trying to add them again in IMEX).

There's nothing special of how those two variables are defined in OV. In fact, any variable that is added as add_definitions(...) or add_compile_definitions(...) prior calling FetchContent_Declare(GC, ...) in OV is being propagated to LLVM_DEFINITIONS.

To fix this, I've added a logic that goes through every variable in LLVM_DEFINITIONS and checks if it was already defined.

p.s.
This problem is not IMEX specific. In GC the OV variables are also propagated to LLVM_DEFINITIONS. However in GC we're adding LLVM_DEFINITIONS as compile options rather than as compile definitions, which works just fine.

@AndreyPavlenko @kurapov-peter is there a better fix for this or we can merge this one?

Signed-off-by: dchigarev <dmitry.chigarev@intel.com>
@AndreyPavlenko
Copy link
Contributor

What about just unsetting this variable, like it's done for this one https://github.com/slyalin/openvino/blob/mlir/cmake/graph-compiler.cmake#L24 ?

@dchigarev
Copy link
Contributor Author

What about just unsetting this variable, like it's done for this one https://github.com/slyalin/openvino/blob/mlir/cmake/graph-compiler.cmake#L24 ?

good point, but I'm not sure about the scalability of this approach. With the current cmake params that I use to build OV there are only two variables that are defined prior FetchContent(gc...), but what if I change my cmake config and then it would become 3-4 or even more variables that I have to temporarily unset and then set again?

Copy link

@kurapov-peter kurapov-peter left a comment

Choose a reason for hiding this comment

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

From the symptoms, this should be fixed in all components by narrowing the scope of definitions to appropriate targets.

@AndreyPavlenko
Copy link
Contributor

Is this issue occurs because find_package() is called twice in the same scope? Can we just do not call it if LLVM_DEFINITIONS is already defined?

@dchigarev
Copy link
Contributor Author

Is this issue occurs because find_package() is called twice in the same scope? Can we just do not call it if LLVM_DEFINITIONS is already defined?

No, it not appears to be the problem.

The tricky part is how llvm defines LLVM_DEFINITIONS variable. It takes all definitions from ${CMAKE_SOURCE_DIR} and appends them to LLVM_DEFINITIONS variable.

Printing ${CMAKE_SOURCE_DIR} in this function shows that it equals to ${OPENVINO_SOURCE_DIR}, meaning that top_dir_definitions variable contains everything that was already defined by openvino (apparently for every target including IMEX). Then if we try to add_compile_definitions(LLVM_DEFINITIONS) in IMEX we would try to define the OV definitions again, which causes the problem.

@dchigarev dchigarev merged commit 25123cc into intel:gc-staging Oct 1, 2024
dchigarev added a commit to dchigarev/mlir-extensions that referenced this pull request Oct 18, 2024
…ntel#898)

[CMakeLists] Filter out already defined LLVM_DEFINITIONS

Signed-off-by: dchigarev <dmitry.chigarev@intel.com>
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.

3 participants