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

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/cmake/modules/CrossCompile.cmake:107
 
+  if (CMAKE_GENERATOR MATCHES "Visual Studio")
+    set(output_path ${output_path}.exe)
----------------
dfukalov wrote:
> smeenai wrote:
> > beanz wrote:
> > > smeenai wrote:
> > > > 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.
> > > Better yet, use `CMAKE_EXECUTABLE_SUFFIX`:
> > > https://cmake.org/cmake/help/latest/variable/CMAKE_EXECUTABLE_SUFFIX.html
> > Will that be set for the host or target platform though?
> > 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 [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/cmake/modules/TableGen.cmake$74 | 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})`.
> 
> 
> AFAIK "Visual Studio" generated projects are working on Windows only, so I just used the same approach as in [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/cmake/modules/TableGen.cmake$74 | 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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107898/new/

https://reviews.llvm.org/D107898