[CMake] Fix recompile all .inc files with LLVM_OPTIMIZED_TABLEGEN in Visual Studio.

[dfukalov]

Summary

After some moment VS solution generated with LLVM_OPTIMIZED_TABLEGEN started to
generate all .inc files for each build. The reason was it had
"<path to native tablegen>/llvm-tblgen" without .exe as a dependency.

[smeenai]

llvm/cmake/modules/CrossCompile.cmake

107

Shouldn't this be something like if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")? The .exe suffix will be present whenever your native system is Windows.

[beanz]

llvm/cmake/modules/CrossCompile.cmake

107

Better yet, use CMAKE_EXECUTABLE_SUFFIX:
https://cmake.org/cmake/help/latest/variable/CMAKE_EXECUTABLE_SUFFIX.html

[smeenai]

llvm/cmake/modules/CrossCompile.cmake

107

Will that be set for the host or target platform though?

[dfukalov]

llvm/cmake/modules/CrossCompile.cmake

107

Shouldn't this be something like if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")? The .exe suffix will be present whenever your native system is Windows.

AFAIK "Visual Studio" generated projects are working on Windows only, so I just used the same approach as in TableGen.cmake.

On the other hand, I've just re-checked ninja and make builds on Windows/Cygwin and they doesn't have the issue with re-generating all .inc files for any build although they have dependencies pointed to llvm-tblgen binary without the extension.

Better yet, use CMAKE_EXECUTABLE_SUFFIX:
https://cmake.org/cmake/help/latest/variable/CMAKE_EXECUTABLE_SUFFIX.html

Thanks for suggestion! It seems the better way is to use set(output_path ${output_path}${CMAKE_EXECUTABLE_SUFFIX}) unconditionally.

Will that be set for the host or target platform though?

I guess we're here about host (in this particular case NATIVE) and the path is returned in output_path_var variable from this build_native_tool(). E.g. in TableGen.cmake it is called as build_native_tool(${target} ${project}_TABLEGEN_EXE DEPENDS ${target}).

[dfukalov]

Using CMAKE_EXECUTABLE_SUFFIX, removed condition.

[beanz]

llvm/cmake/modules/CrossCompile.cmake

107

AFAIK "Visual Studio" generated projects are working on Windows only, so I just used the same approach as in TableGen.cmake.

The big thing to note here is that you're making an apples-to-oranges comparison. That change is actually a change directly related to how MSBuild does something, so it is MSBuild/Visual Studio-specific.

This change is specific to anywhere CMAKE_EXECUTABLE_SUFFIX is used (mostly Windows, but any generator).

I would expect the issue you experienced to appear when using Visual Studio's built-in CMake support which generates Ninja build files under the hood rather than MSBuild files.

Will that be set for the host or target platform though?

This is a good point, and it will get mixed up cross-compiling...

It may be worth defining LLVM_HOST_EXECUTABLE_SUFFIX in config-ix.cmake that we could use in these cases. There are platforms other than windows that use executable suffixes, so we should probably plan for a more robust solution.

[beanz]

Here's a little terrible blurb of CMake that should actually be a very portable solution to this:

function(llvm_get_host_prefixes_and_suffixes)
  message("CMAKE_HOST_SYSTEM_NAME: ${CMAKE_HOST_SYSTEM_NAME}")
  include(Platform/${CMAKE_HOST_SYSTEM_NAME} OPTIONAL RESULT_VARIABLE _includedFile)
  message("_includedFile: ${_includedFile}")
  if (_includedFile)
    set(LLVM_HOST_STATIC_LIBRARY_PREFIX ${CMAKE_STATIC_LIBRARY_PREFIX} PARENT_SCOPE)
    set(LLVM_HOST_STATIC_LIBRARY_SUFFIX ${CMAKE_STATIC_LIBRARY_SUFFIX} PARENT_SCOPE)
    set(LLVM_HOST_SHARED_LIBRARY_PREFIX ${CMAKE_SHARED_LIBRARY_PREFIX} PARENT_SCOPE)
    set(LLVM_HOST_SHARED_LIBRARY_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX} PARENT_SCOPE)
    set(LLVM_HOST_IMPORT_LIBRARY_PREFIX ${CMAKE_IMPORT_LIBRARY_PREFIX} PARENT_SCOPE)
    set(LLVM_HOST_IMPORT_LIBRARY_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX} PARENT_SCOPE)
    set(LLVM_HOST_EXECUTABLE_SUFFIX ${CMAKE_EXECUTABLE_SUFFIX} PARENT_SCOPE)
    set(LLVM_HOST_LINK_LIBRARY_SUFFIX ${CMAKE_LINK_LIBRARY_SUFFIX} PARENT_SCOPE)
  endif()
endfunction()

llvm_get_host_prefixes_and_suffixes()

If you add this blurb into llvm's config-ix.cmake, you can use LLVM_HOST_EXECUTABLE_SUFFIX the same way you just updated the patch to use CMAKE_EXECUTABLE_SUFFIX, and it should be portable for cross-compilation too.

[dfukalov]

In D107898#2941701, @beanz wrote:

Here's a little terrible blurb of CMake that should actually be a very portable solution to this:
...
If you add this blurb into llvm's config-ix.cmake, you can use LLVM_HOST_EXECUTABLE_SUFFIX the same way you just updated the patch to use CMAKE_EXECUTABLE_SUFFIX, and it should be portable for cross-compilation too.

Thanks for the code, the variables look helpful for future use in cross-compile.
Am I right that two message() are just temporary traces and should be removed?

[beanz]

In D107898#2943273, @dfukalov wrote:

Am I right that two message() are just temporary traces and should be removed?

Ah! Yes, sorry those were me debugging it :)

[dfukalov]

Using introduced LLVM_HOST_EXECUTABLE_SUFFIX variable to add the extension for native binary.

[beanz]

LGTM

llvm/cmake/config-ix.cmake

694 ↗

(On Diff #366571)

I realize this was in the code I gave you but, please strip out these two message lines.

[dfukalov]

Fix typo.

[dfukalov]

llvm/cmake/config-ix.cmake

694 ↗

(On Diff #366571)

Thanks for double-check, don't know why I left it here...

[smeenai]

This is breaking our cross-compilation from Linux to Windows. It doesn't seem like the CMake Linux platform files (at least on 3.20.3) define these variables explicitly (I guess they're just relying on them being implicitly empty), so we still see .exe as our CMAKE_EXECUTABLE_SUFFIX, which breaks running tblgen.

Not sure what the best solution is for that.

[beanz]

We should be able to unset them in the function scope, which should do the right thing.

[dfukalov]

In D107898#2956403, @smeenai wrote:

This is breaking our cross-compilation from Linux to Windows. It doesn't seem like the CMake Linux platform files (at least on 3.20.3) define these variables explicitly (I guess they're just relying on them being implicitly empty), so we still see .exe as our CMAKE_EXECUTABLE_SUFFIX, which breaks running tblgen.

Not sure what the best solution is for that.

Can you please provide cmake config parameters (and possible toolchain description) to reproduce the issue?

In D107898#2956809, @beanz wrote:

We should be able to unset them in the function scope, which should do the right thing.

Do you mean something like

if (CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
   set(output_path ${output_path}${LLVM_HOST_EXECUTABLE_SUFFIX})
endif()

in function(build_native_tool target output_path_var)?

[smeenai]

In D107898#2956809, @beanz wrote:

We should be able to unset them in the function scope, which should do the right thing.

Ah, good point.

In D107898#2957018, @dfukalov wrote:

In D107898#2956403, @smeenai wrote:

This is breaking our cross-compilation from Linux to Windows. It doesn't seem like the CMake Linux platform files (at least on 3.20.3) define these variables explicitly (I guess they're just relying on them being implicitly empty), so we still see .exe as our CMAKE_EXECUTABLE_SUFFIX, which breaks running tblgen.

Not sure what the best solution is for that.

Can you please provide cmake config parameters (and possible toolchain description) to reproduce the issue?

In D107898#2956809, @beanz wrote:

We should be able to unset them in the function scope, which should do the right thing.

Do you mean something like

if (CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
   set(output_path ${output_path}${LLVM_HOST_EXECUTABLE_SUFFIX})
endif()

in function(build_native_tool target output_path_var)?

Nope, just emptying out the variables before accessing them.

The config requires access to the Windows SDKs, so it's not the easiest thing to repro, but I can put up a diff to fix it (using @beanz's suggestion).

[smeenai]

Fixing in D108473.

llvm/cmake/modules/CrossCompile.cmake

function(build_native_tool target output_path_var)
   cmake_parse_arguments(ARG "" "" "DEPENDS" ${ARGN})
 
   if(CMAKE_CONFIGURATION_TYPES)
     set(output_path "${${PROJECT_NAME}_NATIVE_BUILD}/Release/bin/${target}")
   else()
     set(output_path "${${PROJECT_NAME}_NATIVE_BUILD}/bin/${target}")
   endif()
 
+  if (CMAKE_GENERATOR MATCHES "Visual Studio")
+    set(output_path ${output_path}.exe)
+  endif()
+
   llvm_ExternalProject_BuildCmd(build_cmd ${target} ${${PROJECT_NAME}_NATIVE_BUILD}
                                 CONFIGURATION Release)
   add_custom_command(OUTPUT "${output_path}"
                      COMMAND ${build_cmd}
                      DEPENDS CONFIGURE_${PROJECT_NAME}_NATIVE ${ARG_DEPENDS}
                      WORKING_DIRECTORY "${${PROJECT_NAME}_NATIVE_BUILD}"
                      COMMENT "Building native ${target}..."
                      USES_TERMINAL)
   set(${output_path_var} "${output_path}" PARENT_SCOPE)
 endfunction()