[PATCH] D139644: [InlineAdvisor] Allow loading advisors as plugins

[Theodoros Theodoridis via Phabricator via llvm-commits]

ttheodor added a comment.

In D139644#3999600 <https://reviews.llvm.org/D139644#3999600>, @mtrofin wrote:

> In D139644#3999423 <https://reviews.llvm.org/D139644#3999423>, @IBricchi wrote:
>
>> We cleaned up the cmake file so that it now compiles all the unit test under unified target and only setting the test plugin implementation as an optional source, and does the opposite for the test plugin target
>
> OK, but that doesn't explain why it'd want to do the opposite for the test plugin target.

Without setting  `LLVM_OPTIONAL_SOURCES` before `add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp)`  we get the following error:

  CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
    Found unknown source file AliasAnalysisTest.cpp
  
    Please update
    /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt
  
  Call Stack (most recent call first):
    cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
    cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
    cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
    unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)
  
  
  CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
    Found unknown source file AliasSetTrackerTest.cpp
  
    Please update
    /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt
  
  Call Stack (most recent call first):
    cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
    cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
    cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
    unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)
  
  
  CMake Error at cmake/modules/LLVMProcessSources.cmake:114 (message):
    Found unknown source file AssumeBundleQueriesTest.cpp
  
    Please update
    /home/theo/llvm-project/llvm/unittests/Analysis/CMakeLists.txt
  
  Call Stack (most recent call first):
    cmake/modules/LLVMProcessSources.cmake:63 (llvm_check_source_file_list)
    cmake/modules/AddLLVM.cmake:489 (llvm_process_sources)
    cmake/modules/AddLLVM.cmake:844 (llvm_add_library)
    unittests/Analysis/CMakeLists.txt:85 (add_llvm_library)
  
  ...
  #Continues listing all other source files in the directory

The issue is that the `add_llvm_library` expects that all source files in the current directory are used by its target; it does this via `llvm_check_source_file_list` by scanning the current directory and checking the target's sources. `llvm_check_source_file_list` ignores files in  `LLVM_OPTIONAL_SOURCES` and is meant for `CMakeLists.txt` that define multiple targets (other unit tests that define such plugins also do this). That's why we need to set all other files as optional sources before adding the plugin module.

In D139644#3999600 <https://reviews.llvm.org/D139644#3999600>, @mtrofin wrote:

> Also, my suggestion was to do that change - i.e. the change that makes conditional compilation of certain files (except for the to-be-introduced plugin unit test) in a separate patch, which would have to land first. This is so that the concerns are separated: one thing is about having a different build behavior on certain platforms. The other is adding the new unit test.

The only conditionally built target is the plugin module, everything else is built normally as before. Note that `LLVM_OPTIONAL_SOURCES` is set after `add_llvm_unittest_with_input_files(AnalysisTests ...`, so the original unit test is not affected.


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

https://reviews.llvm.org/D139644