This is an archive of the discontinued LLVM Phabricator instance.

cmake: Prefix Polly options with LLVM_ to avoid variable shadowing
ClosedPublic

Authored by grosser on May 4 2016, 1:42 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

grosser updated this revision to Diff 56106.May 4 2016, 1:42 AM
grosser retitled this revision from to cmake: Prefix Polly options with LLVM_ to avoid variable shadowing.
grosser updated this object.
grosser added reviewers: bogner, Meinersbur.
grosser added subscribers: llvm-commits, pollydev.
Meinersbur edited edge metadata.May 4 2016, 5:25 AM

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 ↗(On Diff #56106)

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

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.

This revision was automatically updated to reflect the committed changes.

This Differential does a bit more than renaming the cmake variables, which I think it should be reflected in the commit message/title.

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.