Page MenuHomePhabricator

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

Authored by dfukalov on Aug 11 2021, 5:54 AM.

Details

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.

Diff Detail

Event Timeline

dfukalov created this revision.Aug 11 2021, 5:54 AM
dfukalov requested review of this revision.Aug 11 2021, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 5:54 AM
smeenai added inline comments.Aug 11 2021, 9:13 PM
llvm/cmake/modules/CrossCompile.cmake
108

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 added inline comments.Aug 11 2021, 10:02 PM
llvm/cmake/modules/CrossCompile.cmake
108
smeenai added inline comments.Aug 11 2021, 10:03 PM
llvm/cmake/modules/CrossCompile.cmake
108

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

dfukalov marked an inline comment as done.Aug 12 2021, 5:40 AM
dfukalov added inline comments.
llvm/cmake/modules/CrossCompile.cmake
108

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 updated this revision to Diff 365989.Aug 12 2021, 6:20 AM

Using CMAKE_EXECUTABLE_SUFFIX, removed condition.

beanz added inline comments.Aug 12 2021, 6:45 AM
llvm/cmake/modules/CrossCompile.cmake
108

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 added a comment.Aug 12 2021, 9:20 AM

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.

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 added a comment.Aug 13 2021, 9:43 AM

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

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

dfukalov updated this revision to Diff 366571.Aug 16 2021, 2:15 AM

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

beanz added a comment.Aug 16 2021, 2:14 PM

LGTM

llvm/cmake/config-ix.cmake
695

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

dfukalov marked an inline comment as done.Aug 17 2021, 12:24 AM
dfukalov added inline comments.
llvm/cmake/config-ix.cmake
695

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

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2021, 12:25 AM
This revision was automatically updated to reflect the committed changes.
dfukalov marked an inline comment as done.

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.

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

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?

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)?

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

Ah, good point.

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?

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).