Page MenuHomePhabricator

[mlir] Drop customization hooks from StandardToLLVM conversion
ClosedPublic

Authored by ftynse on Fri, Jan 31, 10:54 AM.

Details

Summary

These hooks were originally introduced to support passes deriving the
StandardToLLVM conversion, in particular converting types from different
dialects to LLVM types in a single-step conversion. They are no longer in use
since the pass and conversion infrastructure has evolved sufficiently to make
defining new passes with exactly the same functionality simple through the use
of populate* functions, conversion targets and type converters. Remove the
hooks. Any users of this hooks can call the dialect conversion infrastructure
directly instead, which is likely to require less LoC than these hooks.

Diff Detail

Event Timeline

ftynse created this revision.Fri, Jan 31, 10:54 AM

Unit tests: pass. 62372 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

rriddle accepted this revision.Fri, Jan 31, 11:19 AM
rriddle added inline comments.
mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h
17–18

nit: Can we just remove this comment now?

This revision is now accepted and ready to land.Fri, Jan 31, 11:19 AM

Nice cleanup, thanks Alex!

Alex, Thanks for this! It will definitely make things simpler! However, it's going to conflict with D72802. Could we wait for that diff to land and see what else is needed here to keep everything?

Alex, Thanks for this! It will definitely make things simpler! However, it's going to conflict with D72802. Could we wait for that diff to land and see what else is needed here to keep everything?

Sure, I'll wait for you to land and will rebase. That's why I wanted to put this on your radar.

dcaballe accepted this revision.Fri, Jan 31, 4:00 PM

Just wondering how this will impact the LLVM lowering customization scalability problem in general. By removing the hook for patterns, won't we go from an explosion of populate* functions to an explosion of LLVM lowering passes?

flaub added a subscriber: flaub.Sat, Feb 1, 1:24 AM

I'm wondering how it will be possible to extend the list of patterns for non-standard ops and still make use of the existing LLVMLoweringPass? I'm currently using the patternListFiller to add my own patterns to the pass. If the LLVMLoweringPass was exposed in a header, I could at least reuse that.

flaub added a comment.Sat, Feb 1, 1:27 AM

Note: the issue I'm running into is being able to write lit tests that uses the LLVMLoweringPass along with some extra custom patterns. I was able to do that with this little bit of code:

static PassRegistration<Pass> pass("convert-stdx-to-llvm", "Convert stdx to the LLVM dialect", []() {
  return createLowerToLLVMPass([](auto& converter, auto& patterns) {
    populateStdToLLVMConversionPatterns(converter, patterns);
    populateStdXToLLVMConversionPatterns(converter, patterns);
  });
});
ftynse added a comment.Mon, Feb 3, 4:16 AM

Just wondering how this will impact the LLVM lowering customization scalability problem in general. By removing the hook for patterns, won't we go from an explosion of populate* functions to an explosion of LLVM lowering passes?

This looks orthogonal to me. This will not prevent the potential explosion of populate* functions, neither will the presence of the hooks since you'd need to create one composed "populate" function anyway.

ftynse added a comment.Mon, Feb 3, 4:23 AM

I'm wondering how it will be possible to extend the list of patterns for non-standard ops and still make use of the existing LLVMLoweringPass? I'm currently using the patternListFiller to add my own patterns to the pass. If the LLVMLoweringPass was exposed in a header, I could at least reuse that.

We are talking about ~20 lines of boilerplate vs conceptual complexity of having extension hooks + depending on the decisions std-to-llvm conversion takes to create the pass... Practically, your pass currently inherits a bunch of command-line flags that are specific to std-to-llvm, and it should not. On the higher level, we could expose an LLVMConversionPassBase somewhere, or expose individual patterns like Diego suggested. We might go all the way up to having a ConversionPassBase.

ftynse marked an inline comment as done.Mon, Feb 3, 4:25 AM
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62414 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.