[llvm] r268048 - cmake: Set LINK_POLLY_INTO_TOOLS to ON (v2)

[Tobias Grosser via llvm-commits]

On 05/01/2016 10:02 PM, Justin Bogner wrote:
> Tobias Grosser via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> Author: grosser
>> Date: Fri Apr 29 10:07:22 2016
>> New Revision: 268048
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=268048&view=rev
>> Log:
>> cmake: Set LINK_POLLY_INTO_TOOLS to ON (v2)
>>
>> This is the second try. This time we disable this feature if no Polly checkout
>> is available. For this to work we need to check if tools/polly is present
>> early enough that our decision is known before cmake generates Config/config.h.
>>
>> With Polly checked into LLVM it was since a long time possible to compile
>> clang/opt/bugpoint with Polly support directly linked in, instead of only
>> providing Polly as a separate loadable module. This commit switches the
>> default from providing Polly as a module to linking Polly into tools, such
>> that it becomes unnecessary to load the Polly module when playing with Polly.
>> Such configuration has shown a lot more convenient for day-to-day Polly use.
>>
>> This change does not impact the default behavior of any tool, if Polly is not
>> explicitly enabled when calling clang/opt/bugpoint Polly does not affect
>> compilation.
>>
>> This change also does not impact normal LLVM/clang checkouts that do not
>> contain Polly.

Hi Justin,

thanks for reporting.

> I'm not totally sure that this works correctly. I'm seeing some weird
> behaviour in clean builds, at the very least. It seems like this
> *sometimes* fails and tries to link polly into bugpoint even if you
> don't have it checked out, but I'm having trouble reproducing so I might
> be confused.

This is definitely surprising and unexpected. Do you still see this?
Does this same behavior arise consistently after cmake has built the
make/ninja files or do we have some non-deterministic behavior in a
parallel make/ninja build?

>> +if(WITH_POLLY)
>> +  if(NOT EXISTS ${LLVM_MAIN_SRC_DIR}/tools/polly/CMakeLists.txt)
>> +    set(WITH_POLLY OFF)
>> +  endif()
>> +endif(WITH_POLLY)
>> +
>> +if (NOT WITH_POLLY)
>> + set(LINK_POLLY_INTO_TOOLS OFF)
> 
> Is this usage of set() correct? IIUC it creates a new non-cached
> variable that happens to have the same name as the option above. At the
> very least, I see both WITH_POLLY and LINK_POLLY_INTO_TOOLS set to on in
> my CMakeCache after configuring with this, despite not having polly
> checked out.

I think this is correct. The behavior intended was that the user facing
options are not changed (in case the user checks out Polly later on),
but to ignore these options and to disable the Polly build in case Polly
has not been checked out. As the newly created non-cache variable will
hide the older cache variable [2] this seems to be exactly what is
happening.

Now, I agree that this is clearly unintuitive. I will see if I can come
up with something better and post it for review.

> I don't really know how option() works compared to set(... CACHED) though.

I could not find any specific point of documentation, but from the cmake
mailing list [1] it seems that option(VAR "") is effectively the same as
SET(VAR OFF CACHE BOOL "...").

>> +endif (NOT WITH_POLLY)
>> +
>>  # You can configure which libraries from LLVM you want to include in the
>>  # shared library by setting LLVM_DYLIB_COMPONENTS to a semi-colon delimited
>>  # list of LLVM components. All component names handled by llvm-config are valid.
>> @@ -702,12 +712,6 @@ endforeach()
>>  
>>  add_subdirectory(projects)
>>  
>> -if(WITH_POLLY)
>> -  if(NOT EXISTS ${LLVM_MAIN_SRC_DIR}/tools/polly/CMakeLists.txt)
>> -    set(WITH_POLLY OFF)
>> -  endif()
>> -endif(WITH_POLLY)
>> -
>>  if( LLVM_INCLUDE_TOOLS )
>>    add_subdirectory(tools)
>>  endif()
>>
>> Modified: llvm/trunk/tools/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/CMakeLists.txt?rev=268048&r1=268047&r2=268048&view=diff
>> ==============================================================================
>> --- llvm/trunk/tools/CMakeLists.txt (original)
>> +++ llvm/trunk/tools/CMakeLists.txt Fri Apr 29 10:07:22 2016
>> @@ -16,6 +16,10 @@ if(WITH_POLLY)
>>  else()
>>    set(LLVM_TOOL_POLLY_BUILD Off)
>>  endif()
>> +if(NOT LLVM_TOOL_POLL_BUILD)
> 
> typo?
> 
>> +  MESSAGE(No polly)
> 
> This should probably be message(STATUS ...), no?

Sorry, this was some noise that slipped in and got already reverted.

Tobias

[1] https://cmake.org/pipermail/cmake/2011-November/047186.html
[2] https://cmake.org/cmake/help/v3.0/command/set.html