This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Refactor RewritePatternMatcher into a new PatternApplicator class.
ClosedPublic

Authored by rriddle on Jun 16 2020, 7:07 PM.

Details

Summary

This class enables for abstracting more of the details for the rewrite process, and will allow for clients to apply specific cost models to the pattern list. This allows for DialectConversion and the GreedyPatternRewriter to share the same underlying matcher implementation. This also simplifies the plumbing necessary to support dynamic patterns.

Diff Detail

Event Timeline

rriddle created this revision.Jun 16 2020, 7:07 PM
ftynse added inline comments.Jun 17 2020, 9:29 AM
mlir/include/mlir/IR/PatternMatch.h
454

Do we expect null patterns? Otherwise, we may want to consider (const) RewritePattern & as function argument.

465

Let's also mention that this gets called if onSuccess return failure, because technically the pattern did match in that case.

mlir/lib/IR/PatternMatch.cpp
185

Would it be reasonable, performance-wise, to populate this map within the second for (auto &it : patterns) loop below for each root kind separately, then clear it at the end of the loop? We still get the benefit of only computing the benefit once since the same pattern cannot have different root kinds, and get the benefit of smaller memory footprint in this map.

196–197

This will permanently remove the patterns from the applicator. If want to apply a different cost model later, under which these patterns will no longer be impossible to match, we won't be able to. I'm not sure this is desirable.

PatternApplicator sounds like a weird name. PatternApplier / PatterCostApplier / PatterApplyManager ?

rriddle updated this revision to Diff 271423.Jun 17 2020, 11:24 AM
rriddle marked 7 inline comments as done.

Address comments

Thanks for the review Alex.

mlir/include/mlir/IR/PatternMatch.h
465

Changed to not call onFailure if onSuccess fails. This should keep onFailure specific to pattern match failures.

mlir/lib/IR/PatternMatch.cpp
185

Switched to SmallDenseMap. Also added a fast path for size() == 1, which by and large the most common case.

196–197

Fixed, thanks for catching. This won't affect any existing usages, but at some point in the future we may apply more than one cost model to a specific applicator.

PatternApplicator sounds like a weird name. PatternApplier / PatterCostApplier / PatterApplyManager ?

Applicator seems to like the correct english word to use though?

ftynse accepted this revision.Jun 18 2020, 2:13 AM

Great, thanks River!

This revision is now accepted and ready to land.Jun 18 2020, 2:13 AM
nicolasvasilache accepted this revision.Jun 18 2020, 4:34 AM

Thanks for pushing on this @rriddle !

One question that stems from what I see here is whether you have recommendations on debugging.
I can imagine pitfalls related to applying this extra level of indirection and understanding what happens in what order.
Do you have plans for extra debugging tooling?

PatternApplicator sounds like a weird name. PatternApplier / PatterCostApplier / PatterApplyManager ?

Applicator seems to like the correct english word to use though?

The dictionary says it's specifically used for a device that does such and all usage I saw on the web was in that device context; never heard it spoken though.

Thanks for pushing on this @rriddle !

One question that stems from what I see here is whether you have recommendations on debugging.
I can imagine pitfalls related to applying this extra level of indirection and understanding what happens in what order.
Do you have plans for extra debugging tooling?

I do have plans for some additional debug tooling, but I see that as mostly orthogonal to this change. For the end user, this change doesn't really affect the debugging experience realistically. Aside from this, there are several problems with providing pattern debug tooling:

  • We need to pipe in the debug options from somewhere, or provide a global options cache(similar to the context, printer, etc.). This is kind of annoying because the only alternative is to push the options up to every conversion/pattern pass, which would be a lot of unnecessary additional boiler plate that most users don't want/need IMO.
  • The internal workings of our two main drivers are vastly different, meaning that the debug output/format/experience is different for each driver. This makes it difficult to have generalized tooling that integrates nicely, leading to some amount of duplication.

That being said, there are a few things that I *want* to do that the above make more annoying. I currently have plans to add:

  • print-after-all, print-before-all, etc. related functionality for printing in-between patterns.
  • disable-patterns=* that allows for disabling specific patterns
  • I'd also love to have an interactive command line driver for patterns (and passes too really) that users can interact with(i.e., decide if a pattern should be applied, print the current IR, etc.), but I'm not entirely sure how I'd pipe that through ATM.
  • River
This revision was automatically updated to reflect the committed changes.