[PATCH] D98591: [CodeGen] Add extension points for TargetPassConfig::addMachinePasses

[Raoul Gough via Phabricator via llvm-commits]

drti added inline comments.


================
Comment at: llvm/examples/Bye/CMakeLists.txt:10
 if (NOT WIN32)
+  # Remove any -zdefs flag (which disallows undefined symbols in
+  # shared objects) added by the main cmake files on Linux when
----------------
ychen wrote:
> Why this?
On my Linux system at least (Fedora 33) the Bye example fails to build when configured with -DBUILD_SHARED_LIBS=true. Strictly speaking this belongs in a separate review but seeing as I'm fixing it for the new example I thought I'd sneak this one in.

The failures is because -zdefs is enabled via CMAKE_SHARED_LINKER_FLAGS in llvm/cmake/modules/HandleLLVMOptions.cmake - this linker flag effectively introduces the same restriction against undefined symbols in shared objects as applies to DLLs on Windows (where the Bye example is disabled)

Example error:
examples/Bye/CMakeFiles/Bye.dir/Bye.cpp.o:Bye.cpp:vtable for (anonymous namespace)::LegacyBye: error: undefined reference to 'llvm::Pass::getPassName() const'


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:91
+  enum MachinePassExtensionPointTy {
+    MPEP_EarlyAsPossible,
+    MPEP_PreRegAlloc,
----------------
ychen wrote:
> How about reducing these extension points to a list that very likely to be useful? Unlike IR pipeline, the order among machine passes is pretty firm, if there are no concrete use cases for an extension point, it may become dead code, or we would need to move/rename it in the future.
I agree that we should limit the number of extension points, but I think we also should try to cater for a range of possible usages. The list at the moment more-or-less mirrors the virtual function extension points for the TargetPassConfig derived classes, which I thought was a fair compromise. There's a tradeoff here of course, but I would tend to err on the side of having more rather than fewer, simply because it's a high barrier for someone working on out-of-tree code to get an extension point added if necessary, compared to an in-tree change that needs, say, a new virtual function extension point.


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:522
+                          TargetPassConfig::ExtensionFn Fn)
+      :
+
----------------
ychen wrote:
> format
This was auto-formatted this way, either during arcanist processing or to satisfy some lint/format warnings. I'd be hesitant to change this, even though I agree it doesn't look "right".


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:533
+  RegisterTargetExtension(RegisterTargetExtension &&Other)
+      :
+
----------------
ychen wrote:
> format
Also auto-formatted this way.


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:159
 
+  /// If this is an LLVMTargetMachine then return the downcast
+  /// pointer, otherwise return nullptr.
----------------
ychen wrote:
> `asLLVMTargetMachine` is not needed. If you could just `static_cast` to `LLVMTargetMachine` directly since `TargetMachine` object is almost never created.
I've seen quite a few places in the code doing the exact unchecked static_cast you suggest, but I hadn't wanted to follow that pattern myself. I didn't think this base class warrants the full LLVM RTTI support, and the virtual function seemed like an easy solution. I had intended to submit another review to introduce it in the existing use-cases that are simple enough (see, e.g. the related TODO comment in llvm/tools/llvm-exegesis/lib/Assembler.cpp and FIXME in llvm/tools/opt/opt.cpp)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98591