[InlineAdvisor] Restructure advisor plugin unittest cmake

[IBricchi]

Summary

Move the plugin used in the unittest to test Inline Advisor Plugins
into a separate folder to clean up the cmake file for the analysis
tests.

[IBricchi]

Add new line at end of each file

[IBricchi]

This patch addresses the cmake structure concerns mentioned in https://reviews.llvm.org/D139644

[mtrofin]

lgtm, but please also wait for @thakis 's feedback.

Should the other case at https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Passes/CMakeLists.txt follow suit with a similar design?

[thakis]

Thanks, this looks like a big improvement to me :)

llvm/unittests/Analysis/CMakeLists.txt

62

(as mentioned elsewhere, we shouldn't use the _with_input_files variant, but that's unrelated here :) )

[IBricchi]

Could one of you two push it for me, I don't have commit access.

Username: ibricchi
Email: [e-mail]

[Jake-Egan]

Sorry to raise this issue late (due to the holidays), but it seems this change causes a test failure on AIX

shard JSON output does not exist: /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/unittests/Analysis/./AnalysisTests-LLVM-Unit-26345894-43-81.json

https://lab.llvm.org/buildbot/#/builders/214/builds/5000/steps/6/logs/FAIL__LLVM-Unit__81

Do you have any idea why?

[Jake-Egan]

Reverted this patch because it's still failing on the AIX bot.

[IBricchi]

Hi, sorry for the delay.

I see the error but I'm not sure what it could be, would you be able to tag someone who might know more about why this only fails on AXI @Jake-Egan?

Is there any way of testing changes on buildbot so that I can try and try and correct the behaviour?

[Jake-Egan]

It seems this problem is hard to investigate without having access to an AIX machine, so we are looking into it.

[IBricchi]

Perfect, let me know when something comes up

[Jake-Egan]

llvm/unittests/Analysis/CMakeLists.txt

67

llvm/unittests/Analysis/InlineAdvisorPlugin/CMakeLists.txt

6–10

The problem is that AnalysisTests is missing -brtl in the link step after this change. If you move this AIX related code back to the original file it will fix the problem.

[IBricchi]

[IBricchi]

This should address the -brtl failure

[Jake-Egan]

llvm/unittests/Analysis/CMakeLists.txt

67–68

Is the WIN32 check not redundant when we check for AIX on the next line? Other than that you should be good to commit again, thanks!

[IBricchi]

I'm not 100% sure on the overlap between the two conditions, I just went off what was there before I changed anything. But I've ammended the code to only check for AIX.

[IBricchi]

I don't have commit access, but if this looks good to you @Jake-Egan would you be able to commit the fix.

ibricchi <[e-mail]>

llvm/unittests/Analysis/CMakeLists.txt

set(ANALYSIS_TEST_SOURCES
   TensorSpecTest.cpp
   TBAATest.cpp
   UnrollAnalyzerTest.cpp
   ValueLatticeTest.cpp
   ValueTrackingTest.cpp
   VectorUtilsTest.cpp
   )
 
-# The unit tests target does not use InlineAdvisorPlugin.cpp
-# so if not added to LLVM_OPTIONAL_SOURCES, FileCheck will
-# complain about unused file.
-set(LLVM_OPTIONAL_SOURCES InlineAdvisorPlugin.cpp)
-
 set(MLGO_TESTS TFUtilsTest.cpp TrainingLoggerTest.cpp)
 if (LLVM_HAVE_TFLITE)
   LIST(APPEND ANALYSIS_TEST_SOURCES ${MLGO_TESTS})
 else()
   LIST(APPEND LLVM_OPTIONAL_SOURCES ${MLGO_TESTS})
 endif()
 
 add_llvm_unittest_with_input_files(AnalysisTests
   ${ANALYSIS_TEST_SOURCES}
   )
 
 target_link_libraries(AnalysisTests PRIVATE LLVMTestingSupport)
 
-# The advisor plugin expects to not link against the Analysis, Support and Core 
-# libraries, but expects them to exist in the process loading the plugin. This 
-# doesn't work with DLLs on Windows (where a shared library can't have undefined
-# references), so just skip this testcase on Windows.
-if (NOT WIN32)
-  # On AIX, enable run-time linking to allow symbols from the plugins shared
-  # objects to be properly bound.
-  if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
-    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl")
-  endif()
-
-  # This plugin is built as a stand-alone plugin, so since we don't use the
-  # ANALYSIS_TEST_SOURCES files, we have to add them to LLVM_OPTIONAL_SOURCES
-  # so that FileCheck doesn't complain about unsued files.
-  LIST(APPEND LLVM_OPTIONAL_SOURCES ${ANALYSIS_TEST_SOURCES})
-
-  unset(LLVM_LINK_COMPONENTS)
-  add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp)
-  # Put PLUGIN next to the unit test executable.
-  set_output_directory(InlineAdvisorPlugin
-      BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
-      LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}
-      )
-  set_target_properties(InlineAdvisorPlugin PROPERTIES FOLDER "Tests")
-
-  export_executable_symbols_for_plugins(AnalysisTests)
-  # The plugin depends on some of the output files of intrinsics_gen, so make sure
-  # it is built before the plugin.
-  add_dependencies(InlineAdvisorPlugin intrinsics_gen)
-  add_dependencies(AnalysisTests InlineAdvisorPlugin)
-  set_property(TARGET InlineAdvisorPlugin PROPERTY FOLDER "Tests/UnitTests/AnalysisTests")
-endif()
+add_subdirectory(InlineAdvisorPlugin)

llvm/unittests/Analysis/InlineAdvisorPlugin/CMakeLists.txt

+# The advisor plugin expects to not link against the Analysis, Support and Core 
+# libraries, but expects them to exist in the process loading the plugin. This 
+# doesn't work with DLLs on Windows (where a shared library can't have undefined
+# references), so just skip this testcase on Windows.
+if (NOT WIN32)
+  # On AIX, enable run-time linking to allow symbols from the plugins shared
+  # objects to be properly bound.
+  if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl")
+  endif()
+
+  unset(LLVM_LINK_COMPONENTS)
+  add_llvm_library(InlineAdvisorPlugin MODULE BUILDTREE_ONLY InlineAdvisorPlugin.cpp)
+  # Put PLUGIN next to the unit test executable.
+  set_output_directory(InlineAdvisorPlugin
+      BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/../
+      LIBRARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/../
+      )
+  set_target_properties(InlineAdvisorPlugin PROPERTIES FOLDER "Tests")
+
+  export_executable_symbols_for_plugins(AnalysisTests)
+  # The plugin depends on some of the output files of intrinsics_gen, so make sure
+  # it is built before the plugin.
+  add_dependencies(InlineAdvisorPlugin intrinsics_gen)
+  add_dependencies(AnalysisTests InlineAdvisorPlugin)
+  set_property(TARGET InlineAdvisorPlugin PROPERTY FOLDER "Tests/UnitTests/AnalysisTests")
+endif()