[PATCH] D35258: [Plugins] Add a slim plugin API to work together with the new PM

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

This is looking really awesome. Starting to get into more mundane / nit picky comments below.



================
Comment at: include/llvm/Passes/PassPlugin.h:66-67
+
+  /// Get the \c PassPluginLibraryInfo information of the plugin.
+  const PassPluginLibraryInfo &getInfo() const { return Info; }
+
----------------
Rather than expose the info object, maybe expose nice APIs for everything that wrap it up?


================
Comment at: include/llvm/Passes/PassPlugin.h:93
+/// ```
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT
+llvmGetPassPluginInfo();
----------------
I don't know anything about Windows or DLLs really... But I would naively expect that *here* we want `__declspec(dllimport)`, and we want plugins to use `__declspec(dllexport)`. Is that not the case? I'm happy to just be told "nope, this is how it works" without even a comment. Mostly asking to learn.


================
Comment at: lib/Passes/PassPlugin.cpp:20-21
+  if (!Library.isValid())
+    return make_error<StringError>("Could not load library '" + Filename + "'",
+                                   inconvertibleErrorCode());
+
----------------
Bundle the error string provided by `getPermanentLibrary` here?


================
Comment at: lib/Passes/PassPlugin.cpp:20-21
+  if (!Library.isValid())
+    return make_error<StringError>("Could not load library '" + Filename + "'",
+                                   inconvertibleErrorCode());
+
----------------
chandlerc wrote:
> Bundle the error string provided by `getPermanentLibrary` here?
This will be substantially more efficient if you cast the first component to a `Twine` as then the +s will be saved and evaluated at the end. Something like: `Twine("foo") + a + "bar" + c` is ideal when dealing with `Twine` operands (and that is how `StringError` is built).


================
Comment at: lib/Passes/PassPlugin.cpp:38-40
+        "Wrong API version on plugin '" + Filename + "'. Got version " +
+            std::to_string(P.Info.APIVersion) + ", supported version is " +
+            std::to_string(LLVM_PLUGIN_API_VERSION) + ".",
----------------
Here in particular using the `Twine` composition is likely especially good. You should be able to either directly pass the C-strings or build StringRef's around them and avoid any allocation at this layer.


================
Comment at: tools/opt/NewPMDriver.cpp:31
+#include "llvm/Passes/PassPlugin.h"
+#include "llvm/Passes/PassPluginLoader.h"
 #include "llvm/Support/CommandLine.h"
----------------
Stale header.


================
Comment at: tools/opt/NewPMDriver.cpp:218
 
+  // Call pass plugins
+  for (auto &PluginFN : PassPlugins) {
----------------
Expand a bit? "Walk the pass plugins and let them register any pass builder callbacks." or some such.


================
Comment at: tools/opt/NewPMDriver.cpp:221-223
+    if (!PassPlugin)
+      errs() << "Failed to load passes from '" << PluginFN
+             << "'. Request ignored.\n";
----------------
Generally LLVM style would suggest to continue here rather than using an else. Nto a big deal in this case.


================
Comment at: unittests/Passes/Plugin.cxx:1
+//===- unittests/Passes/Plugins/Plugin.cxx --------------------------------===//
+//
----------------
.cpp?

Also, maybe "TestPlugin"?


================
Comment at: unittests/Passes/PluginsTest.cpp:26-34
+static std::string LibPath(const std::string Name = "TestPlugin") {
+  const std::vector<testing::internal::string> &Argvs =
+      testing::internal::GetArgvs();
+  const char *Argv0 = Argvs.size() > 0 ? Argvs[0].c_str() : "PluginsTests";
+  void *Ptr = (void *)anchor;
+  std::string Path = sys::fs::getMainExecutable(Argv0, Ptr);
+  llvm::SmallString<256> Buf{sys::path::parent_path(Path)};
----------------
Do we have any tests that do this successfully already?

I'm worried it will be a nightmare to get this to work portably. But it is a really awesome level of testing, so seems worth at least trying, just wanted you to be prepared for the potential churn required to get it to work.


================
Comment at: unittests/Passes/PluginsTest.cpp:37
+
+static std::string StdString(const char *c_str) { return c_str ? c_str : ""; }
+
----------------
Use StringRef instead? Or see below...


Repository:
  rL LLVM

https://reviews.llvm.org/D35258