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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D139644#4000660 <https://reviews.llvm.org/D139644#4000660>, @ttheodor wrote:

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

My main question is why you needed to have a target that has only the plugin - I think the answer is that it's because you're building 2 targets now from the same cmake lists, and the second one, the InlineAdvisorPlugin, should just have the plugin, correct?

How about:

- define a variable, e.g. `ANALYSIS_TEST_SOURCES`, that lists all the sources at lines 26-66
- then: `add_llvm_unittest_with_input_files(AnalysisTests ${ANALYSIS_TEST_SOURCES})` and then you don't need to re-list all the sources again (and, thus, avoid maintenance issues), and just concatenate to LLVM_OPTIONAL_SOURCES the whole ANALYSIS_TEST_SOURCE list and please add a comment as to why this happens (the reason above: so you can build the second, stand-alone plugin)

I'd also suggest this second bit happened *only* when you build the plugin, that would make things more clear - so under the line 119.

I think that would both keep the file maintainable - when tests need to be added, they just need to be added to the ANALYSIS_TEST_SOURCE - and clarify that the expansion of LLVM_OPTIONAL_SOURCES only happens if and only if you need to build the extra plugin standalone library.

wdyt?

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





================
Comment at: llvm/unittests/Analysis/CMakeLists.txt:19
 
+# If plugins are disabled, the PluginInlineAdvisorTest will disable itself at runtime. 
+# Otherwise, reconfiguring with plugins disabled will leave behind a stale executable.
----------------
Use `include/llvm/Config/llvm-config.h.cmake` instead define the preprocessor macro.


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

https://reviews.llvm.org/D139644