Before this change certain Polly variables have been used both as a user-facing
CACHED cmake variables as well as a uncached internal variables. Even though
this seems to have worked OK in practice, the behavior only worked due to
one variable shadowing the other. This behavior has been found confusing.
To make the use of cmake variables more clear we now prefix the cached, user
facing variables with LLVM_ as it is common habit for LLVM options. AS a result,
Polly is now enabled with LLVM_BUILD_POLLY instead of BUILD_POLLY and the
linking behavior of Polly is controlled with LLVM_LINK_POLLY_INTO_TOOLS instead
of LINK_POLLY_INTO_TOOLS.
Details
Diff Detail
Event Timeline
This Differential does a bit more than renaming the cmake variables, which I think it should be reflected in the commit message/title.
Each of the LLVM projects usually has its own prefix: "CLANG_", "LLD_", ... . However, this is LLVM's CMakeLists.txt. I therefore suggest to use "LLVM_POLLY_LINK_INTO_TOOLS" and "LLVM_POLLY_BUILD" such that they are grouped together in ccmake/cmake-gui.
Instead of "LLVM_WITH_POLLY" I'd use the already existing "LLVM_TOOL_POLLY_BUILD", or maybe there is an intended difference?
When LLVM_EXTERNAL_*BUILD was renamed to LLVM_TOOL_*_BUILD, the old _BUILD property was still respected in order to allow to switch between older and newer revisions (eg. for bisect). We could do the same here, but personally I don't think it is necessary.
CMakeLists.txt | ||
---|---|---|
349 | It is documented that the Polly directory can be changed using LLVM_EXTERNAL_POLLY_SOURCE_DIR at if(WITH_POLLY) add_llvm_external_project(polly) else() set(LLVM_TOOL_POLLY_BUILD Off) endif() We should either change the documentation or (my preference) actually allow a different Polly source directory. |
As discussed in our phone call, I added another sentence to the commit message to make this clear.
Each of the LLVM projects usually has its own prefix: "CLANG_", "LLD_", ... . However, this is LLVM's CMakeLists.txt. I therefore suggest to use "LLVM_POLLY_LINK_INTO_TOOLS" and "LLVM_POLLY_BUILD" such that they are grouped together in ccmake/cmake-gui.
I used the names you suggested.
Instead of "LLVM_WITH_POLLY" I'd use the already existing "LLVM_TOOL_POLLY_BUILD", or maybe there is an intended difference?
When LLVM_EXTERNAL_*BUILD was renamed to LLVM_TOOL_*_BUILD, the old _BUILD property was still respected in order to allow to switch between older and newer revisions (eg. for bisect). We could do the same here, but personally I don't think it is necessary.
This clearly is worth improving, but as this is more involved, it is probably better to leave this for another patch.
It is documented that the Polly directory can be changed using LLVM_EXTERNAL_POLLY_SOURCE_DIR at
http://llvm.org/docs/CMake.html but I think it is currently impossible it is automatically deactivated by
We should either change the documentation or (my preference) actually allow a different Polly source directory.