[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: CMakeLists.txt:497
+set(LLVM_EXTENSION_LIBS "" CACHE STRING
+	"Semicolon-separated list of extensions to link into tools, or \"all\"")
+
----------------
I feel it would be really nice to add some documentation for this feature, and insert here a pointer here.


================
Comment at: CMakeLists.txt:500
+if(LLVM_ENABLE_PROJECTS STREQUAL "all")
+    set(LLVM_ENABLE_PROJECTS "")
+    foreach(llvm_extension_lib ${LLVM_ALL_EXTENSION_LIBS})
----------------
How can this code path be hit? Line 110 in this file is:

```
if( LLVM_ENABLE_PROJECTS STREQUAL "all" )
  set( LLVM_ENABLE_PROJECTS ${LLVM_ALL_PROJECTS})
endif()
```



================
Comment at: CMakeLists.txt:512
+    string(TOLOWER ${llvm_extension_lib} llvm_extension_dir)
+    include(${LLVM_MAIN_SRC_DIR}/tools/${llvm_extension_dir}/cmake/extension.cmake OPTIONAL)
+endforeach()
----------------
We shouldn't hardcode anything with respect to the "tools" folder in llvm. I see that your patch is setup like if clang is in llvm/tools, but the current recommended setup for laying out the directories for subproject is "side by side" in the monorepo: https://github.com/llvm/llvm-project/


================
Comment at: CMakeLists.txt:521
+endforeach()
+file(APPEND "${CMAKE_BINARY_DIR}/include/llvm/Support/Extension.def" "#undef HANDLE_EXTENSION\n")
 
----------------
Is this file gonna be re-written for each re-execution of CMake?
Will this trigger more recompilation than needed?
If so what can be done about this? Can we generate a copy and replace the final one only if it differs in content? Can we make re-writing the file a rule based on actual dependencies (a target depending on the content of `LLVM_EXTENSION_LIBS` changing)?


================
Comment at: tools/CMakeLists.txt:12
 
-# Build polly before the tools: the tools link against polly when
-# LINK_POLLY_INTO_TOOLS is set.
-if(WITH_POLLY)
-  add_llvm_external_project(polly)
-else()
-  set(LLVM_TOOL_POLLY_BUILD Off)
-endif()
+# Build extensions before the tools: the tools link against extension libs
+foreach(llvm_extension_lib ${LLVM_EXTENSION_LIBS})
----------------
Nit: `Build` doesn't seem the right term here (I know it is here in the original code): the order of build is handled by the dependencies for the targets. I would say "Include" or "Define" maybe?


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

https://reviews.llvm.org/D61446