[PATCH] D140637: [InlineOrder] Plugin Inline Order

[IBricchi via Phabricator via llvm-commits]

IBricchi marked 5 inline comments as done.
IBricchi added a comment.

Thanks for the review! I updated the patch to reflect the comments.



================
Comment at: llvm/include/llvm/Analysis/InlineOrder.h:40-43
+/// An inline order plugin adds a new inline order at runtime by registering
+/// an instance of PluginInlineOrderAnalysis in the current
+/// ModuleAnalysisManager. For example, the following code dynamically registers
+/// the default inline order:
----------------
kazu wrote:
> Sorry, I am slow to understand this.  Is the following correct understanding?
> 
> `PluginInlineOrderAnalysis` is always there, regardless of whether you have a plugin.  If you actually have a plugin, we can use `Factory` in `PluginInlineOrderAnalysis` to construct a custom inline order.
> 
> Now, why are we using an analysis here?  Is that because it's a good hook to use?
An instance of PluginInlineOrderAnalysis only exists if it has been registered, in a normal compilation it won't exist. If it's registered then indeed it's Factory will be used to create a custom order
We use an analysis here because we need the PassBuilder's ability to dynamically load passes as plugins (else we'd need to implement our own custom mechanism).


================
Comment at: llvm/unittests/Analysis/InlineOrderPlugin/InlineOrderPlugin.cpp:60-61
+
+/* New PM Registration */
+llvm::PassPluginLibraryInfo getDefaultDynamicInlineOrderPluginInfo() {
+  return {LLVM_PLUGIN_API_VERSION, "DynamicDefaultInlineOrder",
----------------
kazu wrote:
> I am not familiar with this.  Would you please explain?  The new pass would load the plugin or something?
In the initial commit there was an unnecessary level of indirection, it has been fixed now. The plugin should just register a PluginInlineOrderAnalysis with the ModuleAnalysisManager.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140637