Expose a convenient registerCLOptions() for MlirOptMainConfig

[mehdi_amini]

Summary

This allows for downstream *-opt tools to stay always aligned with the options
exposed by mlir-opt.
It aligns the "generic" options with the more "components" ones like the
pass manager options or the context options.

[rriddle]

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

49

The use of static+External storage makes me feel weird, but seems fine given this situation.

242–244

interleave(registry.getDialectNames(), llvm::outs(), "\n");

?

[mehdi_amini]

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

49

Made this a bit more explicit in a comment.

242–244

Done (note it was pure code motion)

[gflegar]

This seems to cause mlir-opt to hang when passed the --show-dialects option. Ran it through bazel:

bazel run --config=generic_clang @llvm-project//mlir:mlir-opt -- --show-dialects

Though I would be surprised if this is related to the build system.

[gflegar]

Ah, the --show-dialects option is no longer a stand-alone thing, but an additional printout, where you could also do other things. So I guess in my example it's still waiting for some IR as input.

Was this an intentional change?

[mehdi_amini]

In D144782#4171962, @gflegar wrote:

This seems to cause mlir-opt to hang when passed the --show-dialects option. Ran it through bazel:

bazel run --config=generic_clang @llvm-project//mlir:mlir-opt -- --show-dialects

Though I would be surprised if this is related to the build system.

Try ctrl+D after :)

I changed the behavior in that we still process the input file instead of aborting, I can either exit as before or add a message saying "waiting on input, try ctrl+D to abort".

[mehdi_amini]

In D144782#4171985, @gflegar wrote:

Ah, the --show-dialects option is no longer a stand-alone thing, but an additional printout, where you could also do other things. So I guess in my example it's still waiting for some IR as input.

Was this an intentional change?

Indeed it was, I had landed previously https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h#L103 ; but somehow in all my handling of my internal stack of patches didn't reflect it in the command line option description/behavior in the same patch...

[gflegar]

Right, any convenient ways to just get a list of loaded dialects from a script. mlir-opt --show-dialects --version doesn't work (I guess version is processed before we get to --show-dialects), and echo "" | mlir-opt --show-dialects is a weird thing to have.

[mehdi_amini]

Note these are registered dialects and not loaded ones

[gflegar]

Yes, "registered" is what I meant.

But a related question here: what happened to preloadDialectsInContext? This disappeared as a flag, and is now an unused function parameter if I'm reading this correctly? Was this also intended?

[gflegar]

Submitted https://github.com/llvm/llvm-project/commit/f2cdccc034d0696f186f55d6826dd6ac40e4d3d5 for now that restores the preloading behavior, and fixes the test that was failing in our internal configuration. If we really need to support running commands that expect to be given an EOF input to work, we can work on our internal lit runner to support that, but I would prefer to have a heads-up for that before.

[mehdi_amini]

In D144782#4172238, @gflegar wrote:

If we really need to support running commands that expect to be given an EOF input to work, we can work on our internal lit runner to support that, but I would prefer to have a heads-up for that before.

That isn't actually how upstream development work: you have to handle this kind of things in your own fork and not push "anything" upstream for your specificity. Your commit above is just not warranted IMO as it does not fix any upstream problem. Please get reviews in these kind of things!

[gflegar]

In D144782#4172299, @mehdi_amini wrote:

Your commit above is just not warranted IMO as it does not fix any upstream problem.

So you're saying that making preloadDialectInContext an unused parameter was an intentional change?
And that relying on passing empty strings as stdin from lit, even though it's a bug hazard (https://discourse.llvm.org/t/lit-runner-passing-empty-stdin-to-tool-makes-incorrect-tests-pass/65672) is also desired?

From my perspective, both of these were fixing 1-line upstream bugs that were just not detected by upstream testing. If you think they are desired features, feel free to revert my change.

[mehdi_amini]

In D144782#4172374, @gflegar wrote:

In D144782#4172299, @mehdi_amini wrote:

Your commit above is just not warranted IMO as it does not fix any upstream problem.

So you're saying that making preloadDialectInContext an unused parameter was an intentional change?

No it wasn't, thanks for noticing this. But unless a *bot* or a test is actually broken upstream, there is no urgency to push anything and the echo "" | addition seems quite unusual to me.
That is: we need to fix things, either by reverting my patch or through a fix forward, but your downstream situation alone can't be justify an "urgency" from this point of view: we can take the time to iterate on a review and decide what to do (also in general that includes adding tests for the missing coverage).

And that relying on passing empty strings as stdin from lit, even though it's a bug hazard (https://discourse.llvm.org/t/lit-runner-passing-empty-stdin-to-tool-makes-incorrect-tests-pass/65672) is also desired?

From my perspective, the lit behavior is the only environment that define what is and isn't broken upstream. The fact that you have a proprietary emulator of lit that does not match its behavior does not point at a "bug" with a test upstream.
This is independent of defining whether lit current behavior is desired or not.

From my perspective, both of these were fixing 1-line upstream bugs that were just not detected by upstream testing. If you think they are desired features, feel free to revert my change.

I don't think there is an urgency to revert, but note that "fixing bugs" without tests does not seem right in the first place to me. Also if there is no urgency, we likely would want this to be reviewed (by someone who isn't maintaining Google's fork possibly).

mlir/include/mlir/Tools/mlir-opt/MlirOptMain.h

 class DialectRegistry;
 class PassPipelineCLParser;
 class PassManager;
 
 /// Configuration options for the mlir-opt tool.
 /// This is intended to help building tools like mlir-opt by collecting the
 /// supported options.
 /// The API is fluent, and the options are sorted in alphabetical order below.
+/// The options can be exposed to the LLVM command line by registering them
+/// with `MlirOptMainConfig::registerCLOptions();` and creating a config using
+/// `auto config = MlirOptMainConfig::createFromCLOptions();`.
 class MlirOptMainConfig {
 public:
+  /// Register the options as global LLVM command line options.
+  static void registerCLOptions();
+
+  /// Create a new config with the default set from the CL options.
+  static MlirOptMainConfig createFromCLOptions();
+
+  ///
+  /// Options.
+  ///
+
   /// Allow operation with no registered dialects.
   /// This option is for convenience during testing only and discouraged in
   /// general.
   MlirOptMainConfig &allowUnregisteredDialects(bool allow) {
     allowUnregisteredDialectsFlag = allow;
     return *this;
   }
   bool shouldAllowUnregisteredDialects() const {
public:
 
   /// Set whether to run the verifier after each transformation pass.
   MlirOptMainConfig &verifyPasses(bool verify) {
     verifyPassesFlag = verify;
     return *this;
   }
   bool shouldVerifyPasses() const { return verifyPassesFlag; }
 
-private:
+protected:
   /// Allow operation with no registered dialects.
   /// This option is for convenience during testing only and discouraged in
   /// general.
   bool allowUnregisteredDialectsFlag = false;
 
   /// Print the pipeline that will be run.
   bool dumpPassPipelineFlag = false;
 

mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

 #include "mlir/Support/DebugCounter.h"
 #include "mlir/Support/FileUtilities.h"
 #include "mlir/Support/Timing.h"
 #include "mlir/Support/ToolUtilities.h"
 #include "mlir/Tools/ParseUtilities.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/ToolOutputFile.h"
 
 using namespace mlir;
 using namespace llvm;
 
+namespace {
+/// This class is intended to manage the handling of command line options for
+/// creating a *-opt config. This is a singleton.
+struct MlirOptMainConfigCLOptions : public MlirOptMainConfig {
+  MlirOptMainConfigCLOptions() {
+    // These options are static but all uses ExternalStorage to initialize the
+    // members of the parent class. This is unusual but since this class is a
+    // singleton it basically attaches command line option to the singleton
+    // members.
+
+    static cl::opt<bool, /*ExternalStorage=*/true> allowUnregisteredDialects(
+        "allow-unregistered-dialect",
+        cl::desc("Allow operation with no registered dialects"),
+        cl::location(allowUnregisteredDialectsFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> dumpPassPipeline(
+        "dump-pass-pipeline", cl::desc("Print the pipeline that will be run"),
+        cl::location(dumpPassPipelineFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> emitBytecode(
+        "emit-bytecode", cl::desc("Emit bytecode when generating output"),
+        cl::location(emitBytecodeFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> explicitModule(
+        "no-implicit-module",
+        cl::desc("Disable implicit addition of a top-level module op during "
+                 "parsing"),
+        cl::location(useExplicitModuleFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> showDialects(
+        "show-dialects",
+        cl::desc("Print the list of registered dialects and exit"),
+        cl::location(showDialectsFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> splitInputFile(
+        "split-input-file",
+        cl::desc("Split the input file into pieces and process each "
+                 "chunk independently"),
+        cl::location(splitInputFileFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> verifyDiagnostics(
+        "verify-diagnostics",
+        cl::desc("Check that emitted diagnostics match "
+                 "expected-* lines on the corresponding line"),
+        cl::location(verifyDiagnosticsFlag), cl::init(false));
+
+    static cl::opt<bool, /*ExternalStorage=*/true> verifyPasses(
+        "verify-each",
+        cl::desc("Run the verifier after each transformation pass"),
+        cl::location(verifyPassesFlag), cl::init(true));
+
+    static PassPipelineCLParser passPipeline("", "Compiler passes to run", "p");
+    setPassPipelineParser(passPipeline);
+  }
+};
+} // namespace
+
+ManagedStatic<MlirOptMainConfigCLOptions> clOptionsConfig;
+
+void MlirOptMainConfig::registerCLOptions() { *clOptionsConfig; }
+
+MlirOptMainConfig MlirOptMainConfig::createFromCLOptions() {
+  return *clOptionsConfig;
+}
+
 MlirOptMainConfig &MlirOptMainConfig::setPassPipelineParser(
     const PassPipelineCLParser &passPipeline) {
   passPipelineCallback = [&](PassManager &pm) {
     auto errorHandler = [&](const Twine &msg) {
       emitError(UnknownLoc::get(pm.getContext())) << msg;
       return failure();
     };
     if (failed(passPipeline.addToPipeline(pm, errorHandler)))
static LogicalResult processBuffer(raw_ostream &os,
   // matched.
   return sourceMgrHandler.verify();
 }
 
 LogicalResult mlir::MlirOptMain(llvm::raw_ostream &outputStream,
                                 std::unique_ptr<llvm::MemoryBuffer> buffer,
                                 DialectRegistry &registry,
                                 const MlirOptMainConfig &config) {
+  if (config.shouldShowDialects()) {
+    llvm::outs() << "Available Dialects:\n";
+    interleave(registry.getDialectNames(), llvm::outs(), "\n");
+    llvm::outs() << "\n";
+  }
+
   // The split-input-file mode is a very specific mode that slices the file
   // up into small pieces and checks each independently.
   // We use an explicit threadpool to avoid creating and joining/destroying
   // threads for each of the split.
   ThreadPool *threadPool = nullptr;
 
   // Create a temporary context for the sake of checking if
   // --mlir-disable-threading was passed on the command line.
LogicalResult mlir::MlirOptMain(int argc, char **argv, llvm::StringRef toolName,
                                 bool preloadDialectsInContext) {
   static cl::opt<std::string> inputFilename(
       cl::Positional, cl::desc("<input file>"), cl::init("-"));
 
   static cl::opt<std::string> outputFilename("o", cl::desc("Output filename"),
                                              cl::value_desc("filename"),
                                              cl::init("-"));
 
-  static cl::opt<bool> splitInputFile(
-      "split-input-file",
-      cl::desc("Split the input file into pieces and process each "
-               "chunk independently"),
-      cl::init(false));
-
-  static cl::opt<bool> verifyDiagnostics(
-      "verify-diagnostics",
-      cl::desc("Check that emitted diagnostics match "
-               "expected-* lines on the corresponding line"),
-      cl::init(false));
-
-  static cl::opt<bool> verifyPasses(
-      "verify-each",
-      cl::desc("Run the verifier after each transformation pass"),
-      cl::init(true));
-
-  static cl::opt<bool> allowUnregisteredDialects(
-      "allow-unregistered-dialect",
-      cl::desc("Allow operation with no registered dialects (discouraged: testing only!)"), cl::init(false));
-
-  static cl::opt<bool> showDialects(
-      "show-dialects", cl::desc("Print the list of registered dialects"),
-      cl::init(false));
-
-  static cl::opt<bool> emitBytecode(
-      "emit-bytecode", cl::desc("Emit bytecode when generating output"),
-      cl::init(false));
-
-  static cl::opt<bool> explicitModule{
-      "no-implicit-module",
-      cl::desc(
-          "Disable implicit addition of a top-level module op during parsing"),
-      cl::init(false)};
-
-  static cl::opt<bool> dumpPassPipeline{
-      "dump-pass-pipeline", cl::desc("Print the pipeline that will be run"),
-      cl::init(false)};
-
   InitLLVM y(argc, argv);
 
   // Register any command line options.
+  MlirOptMainConfig::registerCLOptions();
   registerAsmPrinterCLOptions();
   registerMLIRContextCLOptions();
   registerPassManagerCLOptions();
   registerDefaultTimingManagerCLOptions();
   DebugCounter::registerCLOptions();
-  PassPipelineCLParser passPipeline("", "Compiler passes to run", "p");
 
   // Build the list of dialects as a header for the --help message.
   std::string helpHeader = (toolName + "\nAvailable Dialects: ").str();
   {
     llvm::raw_string_ostream os(helpHeader);
     interleaveComma(registry.getDialectNames(), os,
                     [&](auto name) { os << name; });
   }
   // Parse pass names in main to ensure static initialization completed.
   cl::ParseCommandLineOptions(argc, argv, helpHeader);
-
-  if (showDialects) {
-    llvm::outs() << "Available Dialects:\n";
-    interleave(
-        registry.getDialectNames(), llvm::outs(),
-        [](auto name) { llvm::outs() << name; }, "\n");
-    return success();
-  }
+  MlirOptMainConfig config = MlirOptMainConfig::createFromCLOptions();
 
   // Set up the input file.
   std::string errorMessage;
   auto file = openInputFile(inputFilename, &errorMessage);
   if (!file) {
     llvm::errs() << errorMessage << "\n";
     return failure();
   }
 
   auto output = openOutputFile(outputFilename, &errorMessage);
   if (!output) {
     llvm::errs() << errorMessage << "\n";
     return failure();
   }
-  // Setup the configuration for the main function.
-  MlirOptMainConfig config;
-  config.setPassPipelineParser(passPipeline)
-      .splitInputFile(splitInputFile)
-      .verifyDiagnostics(verifyDiagnostics)
-      .verifyPasses(verifyPasses)
-      .allowUnregisteredDialects(allowUnregisteredDialects)
-      .preloadDialectsInContext(preloadDialectsInContext)
-      .emitBytecode(emitBytecode)
-      .useExplicitModule(explicitModule)
-      .dumpPassPipeline(dumpPassPipeline);
-
   if (failed(MlirOptMain(output->os(), std::move(file), registry, config)))
     return failure();
 
   // Keep the output file if the invocation of MlirOptMain was successful.
   output->keep();
   return success();
 }