search

ROCm / ROCm-CompilerSupport

The compiler support repository provides various Lightning Compiler related services.
45 stars 31 forks source link

Find Clang avoiding the system (non-ROC) LLVM #16

Closed candrews closed 4 years ago

candrews commented 5 years ago

Use NO_DEFAULT_PATH to avoid finding the system (non-ROC) LLVM. Use /opt/rocm/llvm as an additional hint to be consistent with the build systems of other ROC projects (such as ROCm-OpenCL-Driver)

For a similar change, see https://github.com/RadeonOpenCompute/ROCm-Device-Libs/commit/698cec4790933ecb5e4c1091616f4505134c2b63

scott-linder commented 5 years ago

This LGTM, thank you for the patch. I will apply this in our internal repo soon.

scott-linder commented 5 years ago

I should have re-read the CMake docs before posting. After going ahead and testing this I'm not sure this is what we want. We rely on CMAKE_PREFIX_PATH in order to find the correct copy of LLVM, and NO_DEFAULT_PATH enables NO_CMAKE_PATH+NO_CMAKE_ENVIRONMENT_PATH, which breaks this.

It might be reasonable to set NO_SYSTEM_ENVIRONMENT_PATH and/or NO_CMAKE_SYSTEM_PATH? In any event CMAKE_PREFIX_PATH is the first path searched by CMake, so it is always possible to preempt the default paths. I would rather not replace this generic ability that all components can share with ${LLVM_PATH}.

candrews commented 5 years ago

Perhaps you can confer with the other ROC projects and determine a consistent approach?

Right now, the one proposed in this PR, find_package(Clang REQUIRED CONFIG PATHS ${LLVM_DIR} "/opt/rocm/llvm" NO_DEFAULT_PATH) is the one most commonly used across all ROC projects, FWIW. If this project decides to be different, perhaps it can convince all the others to change too? I <3 consistency.

Regarding CMAKE_PREFIX_PATH, that impacts all uses of find_package, and that's not desirable.

scott-linder commented 5 years ago

I may not understand the intent of CMAKE_PREFIX_PATH, but it seems desirable to me. In what scenario will a build want to reference multiple different copies of LLVM?

Inventing a new cache variable like ${LLVM_DIR} for each package seems to defeat the purpose of having find_package in the first place. Being able to simply populate one common variable in CMAKE_PREFIX_PATH and then write clean CMake like find_package(Clang REQUIRED) seems preferable. However, I also agree that having a consistent approach is important, and I can try to work internally to figure out that approach.

candrews commented 5 years ago

Hello @scott-linder, I'm curious if there's news on this issue - is there anything you can share regarding progress you may have made internally?

Thanks!

scott-linder commented 4 years ago

Hi @candrews, unfortunately I haven't made any progress on this internally yet. I apologize for the delay in replying, I wanted to actually initiate a discussion about this internally before posting, and it took me a while to get to it. Hopefully I will have a better update soon. Thank you again for the patch and your patience!