[DRAFT][TableGen.cmake] Allow optionally to build tablegen in Release and use it in Debug for Ninja Multi-Config generator
#3673
Conversation
…e it in Debug for `Ninja Multi-Config` generator If `Ninja Multi-Config` is used and `LLVM_TABLEGEN_USE_RELEASE_BUILD=YES` is set tablegen binaries will be built in `Release`, even when building for `Debug` configuration, without needing a separate configured build directory just for tablegen binaries.
|
This is a draft to get feedback whether it makes sense to try to get it in upstream. |
|
The other benefit of |
compnerd
left a comment
compnerd
left a comment
There was a problem hiding this comment.
I think that the idea itself is good, but I would like to see this tied into the existing path. Also note that the existing path is very well tested - it is part of the cross-compilation path as well.
| list(TRANSFORM tblgen_includes PREPEND -I) | ||
|
|
||
| set(tablegen_exe ${${project}_TABLEGEN_EXE}) | ||
| set(tablegen_depends ${${project}_TABLEGEN_TARGET} ${tablegen_exe}) |
There was a problem hiding this comment.
Why the dependency on the tablegen target? Seems ... recursive?
There was a problem hiding this comment.
I'm just following what existing code was doing, see comments earlier that start with
We need both _TABLEGEN_TARGET and _TABLEGEN_EXE in the DEPENDS list
There was a problem hiding this comment.
I'd suggest splitting this change (creating two variables and using them in existing places) into a prep commit so it's clear what the function change is.
|
|
||
| set(tablegen_exe ${${project}_TABLEGEN_EXE}) | ||
| set(tablegen_depends ${${project}_TABLEGEN_TARGET} ${tablegen_exe}) | ||
| if (LLVM_TABLEGEN_USE_RELEASE_BUILD) |
There was a problem hiding this comment.
This is something that already is supported without the multi-generator support LLVM_OPTIMIZED_TABLEGEN. I'd rather key off the same option and change the implementation if we are targeting a multi-generator build.
dexonsmith
left a comment
dexonsmith
left a comment
There was a problem hiding this comment.
This'll be great to have. I agree with @compnerd about improving the existing optimized tablegen feature rather than creating a new one; also, I think you should split out some prep commits to make things more clear.
| list(TRANSFORM tblgen_includes PREPEND -I) | ||
|
|
||
| set(tablegen_exe ${${project}_TABLEGEN_EXE}) | ||
| set(tablegen_depends ${${project}_TABLEGEN_TARGET} ${tablegen_exe}) |
There was a problem hiding this comment.
I'd suggest splitting this change (creating two variables and using them in existing places) into a prep commit so it's clear what the function change is.
|
|
||
| # CMake doesn't let compilation units depend on their dependent libraries on some generators. | ||
| if(NOT CMAKE_GENERATOR STREQUAL "Ninja" AND NOT XCODE) | ||
| if(NOT CMAKE_GENERATOR MATCHES "Ninja" AND NOT XCODE) |
There was a problem hiding this comment.
I'd also suggest a separate prep commit for these MATCHES changes.
|
Prep commit posted at https://reviews.llvm.org/D118100 |
3 participants
If
Ninja Multi-Configis used andLLVM_TABLEGEN_USE_RELEASE_BUILD=YESis settablegen binaries will be built in
Release, even when building forDebugconfiguration,without needing a separate configured build directory just for tablegen binaries.
To get such a setup you'd configure like this: