[mlir][core] Fix inline pass default pipeline dump

[wpmed92]

Summary

The inliner pass performs canonicalization when created programtically, run with mlir-opt with default options, or when explicitly specified. However, when running the pipeline resulting from a -dump-pass-pipeline on a default inline pass, the canonicalization is not performed as part of the inlining. This is because the default value for the default-pipeline option of the inline pass is an empty string, and this is selected during the dumping. When InlinerPass::initializeOptions detects the empty string, it sets the defaultPipeline to nullptr, which was previously set to canonicalize in the InlinerPass constructor, thus the canonicalization is not performed.

The added test checks if the inline pass performs canonicalization by default, and that the dumped default-pipeline is set to canonicalize.

Fixes: https://github.com/llvm/llvm-project/issues/60960

[mehdi_amini]

It's not clear to me that this is correct. Actually dumping the default-pipeline does not seem relevant to me, I'm not even sure why is it an option right now?

On the other hand the opPipelineList options is what should contain the actual pipeline.

[mehdi_amini]

Actually there is this code:

auto pipelineIt = pipelines.find(opName);
if (pipelineIt == pipelines.end()) {
  // If a pipeline didn't exist, use the default if possible.
  if (!defaultPipeline)
    return success();

  OpPassManager defaultPM(opName);
  defaultPipeline(defaultPM);
  pipelineIt = pipelines.try_emplace(opName, std::move(defaultPM)).first;
}

in the Inliner, where we apply the default in complement to the opPipelineList, so they are both need to be dumped indeed.

Making canonicalize the default flag here can make sense, but then is this function still useful?

/// This function implements the default inliner optimization pipeline.
static void defaultInlinerOptPipeline(OpPassManager &pm) {
  pm.addPass(createCanonicalizerPass());
}

[wpmed92]

@mehdi_amini

Making canonicalize the default flag here can make sense, but then is this function still useful?

/// This function implements the default inliner optimization pipeline.
static void defaultInlinerOptPipeline(OpPassManager &pm) {
  pm.addPass(createCanonicalizerPass());
}

Although our tests pass without adding canonicalization in static void defaultInlinerOptPipeline(OpPassManager &pm), I found that it is still needed, since programmatic creation (mlir::createInlinerPass) of the InlinerPass does not make use of the default option in Passes.td. So removing static void defaultInlinerOptPipeline(OpPassManager &pm) would cause the canonicalization to not be performed when using mlir::createInlinerPass(). So I think we need both the default value of default-pipeline to be canonicalize, and also adding the canonicalization in defaultInlinerOptPipeline.
cc: @rriddle

[wpmed92]

In D145066#4163107, @mehdi_amini wrote:

It's not clear to me that this is correct. Actually dumping the default-pipeline does not seem relevant to me, I'm not even sure why is it an option right now?

I feel like the strangeness here is that we have the canonicalization by default through defaultInlinerOptPipeline, but the default value of default-pipeline currently does not reflect that, it's an empty string. However, the canonicalization is still performed when invoking the inline pass without options through mlir-opt. This is because if the default-pipeline is not specified in the command line, we leave the defaultPipeline set to canonicalize. But the problem is that if an inline pipeline without options specified is dumped, it is output as such:
builtin.module(inline{default-pipeline= max-iterations=4 })
Applying that will cause the defaultPipeline set to nullptr, thus removing canonicalization. So it essentially means that the output of pipeline dumping, when applied, will behave differently than the pipeline that was dumped. This is why we need the "canonicalize" as default value for default-pipeline option.

[mehdi_amini]

In D145066#4163982, @wpmed92 wrote:

@mehdi_amini

Making canonicalize the default flag here can make sense, but then is this function still useful?

/// This function implements the default inliner optimization pipeline.
static void defaultInlinerOptPipeline(OpPassManager &pm) {
  pm.addPass(createCanonicalizerPass());
}

Although our tests pass without adding canonicalization in static void defaultInlinerOptPipeline(OpPassManager &pm), I found that it is still needed, since programmatic creation (mlir::createInlinerPass) of the InlinerPass does not make use of the default option in Passes.td

This seems unexpected to me. Why wouldn’t the default ctor for the inliner make use of the default from the .td file?

[wpmed92]

In D145066#4164161, @mehdi_amini wrote:

In D145066#4163982, @wpmed92 wrote:

@mehdi_amini

Making canonicalize the default flag here can make sense, but then is this function still useful?

/// This function implements the default inliner optimization pipeline.
static void defaultInlinerOptPipeline(OpPassManager &pm) {
  pm.addPass(createCanonicalizerPass());
}

Although our tests pass without adding canonicalization in static void defaultInlinerOptPipeline(OpPassManager &pm), I found that it is still needed, since programmatic creation (mlir::createInlinerPass) of the InlinerPass does not make use of the default option in Passes.td

This seems unexpected to me. Why wouldn’t the default ctor for the inliner make use of the default from the .td file?

The InlinerBase ctor does make use of it, it populates defaultPipelineStr to the value in the .td file. But the InlinerPass default ctor doesn't use defaultPipelineStr to turn it into defaultPipeline, it's only used in InitializeOptions, where it's parsed with parsePassPipeline:

std::string defaultPipelineCopy = defaultPipelineStr;
defaultPipeline = [=](OpPassManager &pm) {
      (void)parsePassPipeline(defaultPipelineCopy, pm);
};

Should we modify defaultInlinerOptPipeline to parse the pipeline from defaultPipelineStr, the same way as above, so that programmatic creation and usage with mlir-opt both use the same source-of-truth (defaultPipelineStr)?

[mehdi_amini]

Seems like InlinerPass has these two members:

/// An optional function that constructs a default optimization pipeline for
/// a given operation.
std::function<void(OpPassManager &)> defaultPipeline;
/// A map of operation names to pass pipelines to use when optimizing
/// callable operations of these types. This provides a specialized pipeline
/// instead of the default. The vector size is the number of threads used
/// during optimization.
SmallVector<llvm::StringMap<OpPassManager>, 8> opPipelines;

How are they ending up serialized?

[rriddle]

I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480

mlir/test/Pass/inliner-dump-default-pipeline.mlir

1 ↗

(On Diff #501493)

This doesn't look like a test that should be in the Pass/ directory, can we move this to the Transforms/ directory?

4–10 ↗

(On Diff #501493)

Do we even need IR in the test? I would expect it to just check the default pipeline.

[wpmed92]

In D145066#4167514, @rriddle wrote:

I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480

Thank you for reviewing @rriddle! I updated the diff: moved the test to /Transforms, and deleted the IR to test if canonicalize is performed, only testing dump output.
Can you please commit on my behalf?

author: wpmed92
email: [e-mail]

(I will use arcanist for my next patch.)

[mehdi_amini]

In D145066#4167514, @rriddle wrote:

I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480

Will this be enough to solve the entire dump including the members of the InlinerPass class?

[rriddle]

In D145066#4168413, @mehdi_amini wrote:

In D145066#4167514, @rriddle wrote:

I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480

Will this be enough to solve the entire dump including the members of the InlinerPass class?

Yeah, it should. The callable stuff gets killed, and the list is already specified as an option.

[mehdi_amini]

In D145066#4182035, @rriddle wrote:

In D145066#4168413, @mehdi_amini wrote:

In D145066#4167514, @rriddle wrote:

I'd be fine landing this patch for now to fix the issue. The root of the inliner issues should be fixed when I rebase+land https://reviews.llvm.org/D134480

Will this be enough to solve the entire dump including the members of the InlinerPass class?

Yeah, it should. The callable stuff gets killed, and the list is already specified as an option.

OK, great, then this patch is fine with me as well :)

mlir/include/mlir/Transforms/Passes.td

def GenerateRuntimeVerification : Pass<"generate-runtime-verification"> {
   let constructor = "mlir::createGenerateRuntimeVerificationPass()";
 }
 
 def Inliner : Pass<"inline"> {
   let summary = "Inline function calls";
   let constructor = "mlir::createInlinerPass()";
   let options = [
     Option<"defaultPipelineStr", "default-pipeline", "std::string",
-           /*default=*/"", "The default optimizer pipeline used for callables">,
+           /*default=*/"\"canonicalize\"", "The default optimizer pipeline used for callables">,
     ListOption<"opPipelineList", "op-pipelines", "OpPassManager",
                "Callable operation specific optimizer pipelines (in the form "
                "of `dialect.op(pipeline)`)">,
     Option<"maxInliningIterations", "max-iterations", "unsigned",
            /*default=*/"4",
            "Maximum number of iterations when inlining within an SCC">,
   ];
 }

mlir/test/Transforms/inlining-dump-default-pipeline.mlir

+// RUN: mlir-opt %s -pass-pipeline="builtin.module(inline)" -dump-pass-pipeline 2>&1 | FileCheck %s
+// CHECK: builtin.module(inline{default-pipeline=canonicalize max-iterations=4 })