[PATCH] D74588: Use DISABLE_LLVM_LINK_LLVM_DYLIB for TableGenTests

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In D74588#2660144 <https://reviews.llvm.org/D74588#2660144>, @aaronpuchert wrote:

> In D74588#2659652 <https://reviews.llvm.org/D74588#2659652>, @dsanders wrote:
>
>> All I wanted was a subdirectory to organize the sources specific to the GISel tablegen passes :-).
>
> Not only that, you also want to use them in `TableGenTests`, and this I think is why can't just have them in `llvm-tblgen`.

I'd completely forgotten about the CodeExpander tests, sorry about that. Yes, you're right those do require more than just a subdirectory.

>> I'd have been happy with GlobalISel/Foo.cpp in the llvm/utils/TableGen/CMakeLists.txt but CMake objects to that.
>
> I'm sure CMake shouldn't complain about that, the unit test dependency should be the issue.

FWIW, it might have been LLVM specific. What I see in the code for that at the moment would have let it through though

>> I then went with the object library so that CMake would build the objects that I could then link as part of llvm-tblgen instead of being a proper library but the bots running older CMake's objected to that.
>
> That I think should not be a problem anymore.
>
> To summarize: these are sources that are not part of LLVM proper but of a tool, but you want to unit test them, so you need them as a separate target that you can use for both. It appears to be unconventional to have such code unit tested. But I don't want to ask you to rewrite the tests as lit tests. (I have no idea whether that's feasible even.)

It's possible to convert to lit tests but being able to unit test directly saves on the extra boilerplate code. Options like -gicombiner-stop-after-parse are there for testing but they're really just early exits on what it was normally doing. The CodeExpander tests would need boilerplate to wrap it in a command line interface.

> In my view the best solution would be to go with the original object library, which should be fine by now because we have `cmake_minimum_required(VERSION 3.13.4)` in `llvm/CMakeLists.txt`, so everybody has to use a CMake version that supports them. We're not talking about an awful number of sources, and for those not building unit tests it behaves just as if the sources were part of `llvm-tblgen`.

That sounds good to me. Would you like me to change it back to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74588