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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen 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
----------------
Why this?


================
Comment at: llvm/examples/CMakeLists.txt:11
 add_subdirectory(Bye)
+add_subdirectory(MachineExtension)
 
----------------
I think it's better to call this `MachineBye` to reflect that the usage/implementation are similar between the two. If you think it makes sense, make sure the class name is also renamed accordingly.


================
Comment at: llvm/examples/MachineExtension/MachineExtension.cpp:33
+bool ExampleMachinePass::runOnMachineFunction(MachineFunction &MF) {
+  // This example shows deletion of calls to a supposed "_dummy_intrinsic"
+  // function.
----------------
No need for anything concrete there. I think printing some string using `errs()` should be enough.


================
Comment at: llvm/examples/MachineExtension/MachineExtension.cpp:62
+static RegisterTargetExtension Registrar(TargetPassConfig::MPEP_EarlyAsPossible,
+                                         &ExampleMachinePass::selfRegister);
----------------
Better to use lambda for this one-time registration.


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:91
+  enum MachinePassExtensionPointTy {
+    MPEP_EarlyAsPossible,
+    MPEP_PreRegAlloc,
----------------
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.


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:509
+  /// Target at the given extension point.
+  void applyAnyExtensions(MachinePassExtensionPointTy MPEP);
 };
----------------
I'd prefer to call it `addMachinePassExtentions`.


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:522
+                          TargetPassConfig::ExtensionFn Fn)
+      :
+
----------------
format


================
Comment at: llvm/include/llvm/CodeGen/TargetPassConfig.h:533
+  RegisterTargetExtension(RegisterTargetExtension &&Other)
+      :
+
----------------
format


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:159
 
+  /// If this is an LLVMTargetMachine then return the downcast
+  /// pointer, otherwise return nullptr.
----------------
`asLLVMTargetMachine` is not needed. If you could just `static_cast` to `LLVMTargetMachine` directly since `TargetMachine` object is almost never created.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98591