Page MenuHomePhabricator

[MLIR] Debug log IR after pattern applications
ClosedPublic

Authored by frgossen on Apr 21 2021, 3:56 AM.

Details

Summary

Like print-ir-after-all and -before-all, this allows to inspect IR for
debug purposes. While the former allow to inspect only between passes, this
change allows to follow the rewrites that happen within passes.

Diff Detail

Event Timeline

frgossen created this revision.Apr 21 2021, 3:56 AM
frgossen requested review of this revision.Apr 21 2021, 3:56 AM
jpienaar added inline comments.
mlir/lib/Rewrite/PatternApplicator.cpp
42

Why not just have this function defined and referenced only in debug? Same below. So rather than hallowing out, you'd have both only in debug and then yes you'd have two more debug blocks at call site (so 1 here, 2 below vs 2 inside) but seems like it would be clearer that it only has effect in debug mode.

frgossen updated this revision to Diff 339242.Apr 21 2021, 7:59 AM

Address comments

frgossen marked an inline comment as done.Apr 21 2021, 8:00 AM

Thanks!

mlir/lib/Rewrite/PatternApplicator.cpp
42

Did it analogous to logImpossibleToMatch. Changed this for both now :)

rriddle requested changes to this revision.Apr 21 2021, 10:13 AM
rriddle added inline comments.
mlir/lib/Rewrite/PatternApplicator.cpp
42

This looks really awkward, what are you trying to achieve here? This assumption of Module is invalid, not all IR is anchored under a module and when it is the module isn't necessarily the top-level operation.

This revision now requires changes to proceed.Apr 21 2021, 10:13 AM
rriddle added inline comments.Apr 21 2021, 10:15 AM
mlir/lib/Rewrite/PatternApplicator.cpp
42

Dumping the entire module also seems a bit like overkill. Is there a more principled approach that we can have here?

rriddle added inline comments.Apr 21 2021, 10:24 AM
mlir/lib/Rewrite/PatternApplicator.cpp
42

Reading my comments back, they can seem a bit terse. Apologies for that. I'm very +1 on improving the logging here, and dumping IR in-between pattern applications is something that has been discussed for a while. My main reservation is that this seems very stop gap, and this would be a good time to think of something that scales.

frgossen updated this revision to Diff 339516.Apr 22 2021, 1:53 AM
frgossen marked an inline comment as done.

Address comments

frgossen marked an inline comment as done.Apr 22 2021, 3:02 AM
frgossen added inline comments.
mlir/lib/Rewrite/PatternApplicator.cpp
42

Why the module / top level op?

  • Correct me if I'm wrong but in the PatternApplicator there isn't a simple way to know what surrounding ops may be affected by a rewrite. Limiting the IR dump to, e.g., the parent op might unintentionally hide some effects. It would be nice to pick the root op of the currently running pass here but propagating that information would need changes in many places, I think - probably not worth it.
  • I would like to print the IR from the same root op throughout the entire rewrite process. This makes IR dumps comparable and I found it much easier to follow the changes. Again, the current pass' root op would be a good choice.

Something that scales

  • I am not entirely sure what you mean here. The debug log in the dialect conversion, e.g., is implemented in a very similar way.
  • Instead of the LLVM_DEBUG mechanism, we could use MLIRContext flags here. This would also allow to make the IR dumps more configurable, e.g., dump only the parent op, dump the top level op, etc. I'd be happy to implement this if it is what you mean with "something that scales". Otherwise, can you elaborate?
  • Either way, this change is very local and easy enough to replace once something more advanced is in place.

Friendly ping.

mehdi_amini added inline comments.Apr 26 2021, 6:40 PM
mlir/lib/Rewrite/PatternApplicator.cpp
42

Correct me if I'm wrong but in the PatternApplicator there isn't a simple way to know what surrounding ops may be affected by a rewrite. Limiting the IR dump to, e.g., the parent op might unintentionally hide some effects.

You can limit yourself to the next IsolatedFromAbove enclosing operation: passes (and so multi-threading) is limited by this.

56

(I think the else should never be needed here if you update the previous function to look for the first IsolatedFromAbove parent op / or the top-level).

191

Add a comment about why you're getting this here (i.e. op may be invalidated below). This got me to think a little...

rriddle added inline comments.Apr 26 2021, 6:59 PM
mlir/lib/Rewrite/PatternApplicator.cpp
42

By something that scales, I do in fact mean something that is more configurable. My main reservation is that dumping huge chunks of IR while debugging only fits one particular debugging use case, and doesn't really scale to general debugging (especially if you are in a medium or larger module). If I wanted to use my normal debug flow, I'd have to come in here and comment this out (which is relatively the same as what you have to do now when you want to dump it). There has been a TODO for a while (though I've been dragged in many other directions) to have a general "debug config" that gets piped from passes into the different pattern drivers. This config would have different options to aid debugging/testing/etc. For example, disabling/enabling specific patterns by name. I see this kind of IR dumping as fitting in that kind of scheme, i.e. control IR dumping granularity in a similar way to what happens for passes (though these configs would be pass specific).

As for finding a specific root operation to dump from, this could be something provided to the PatternApplicator constructor by the pattern driver. An alternative is to go with @mehdi_amini 's suggestion, which wouldn't necessarily give you the same operation every time, but would give something at a finer grain than the top-level module.

At any rate, maybe my concern isn't that warranted anyways. This debug logging is under a different DEBUG_TYPE than say dialect-conversion, which already kind of gives it that "special flag to enable" kind of feel. That may work for now.

frgossen updated this revision to Diff 340742.Apr 27 2021, 12:45 AM
frgossen marked 4 inline comments as done.

Address comments

frgossen marked 2 inline comments as done.Apr 27 2021, 12:47 AM

Thanks!

rriddle accepted this revision.Apr 27 2021, 1:23 AM

Given that nothing else is really logged with this DEBUG_TYPE, I'm alright with this until we have something better.

This revision is now accepted and ready to land.Apr 27 2021, 1:23 AM
This revision was automatically updated to reflect the committed changes.