This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Debug print pattern before and after matchAndRewrite call
ClosedPublic

Authored by Hardcode84 on Apr 28 2021, 5:06 AM.

Details

Summary

Motivation: we have passes with lot of rewrites and when one one them segfaults or asserts, it is very hard to find waht exactly pattern failed without debug info.

Diff Detail

Event Timeline

Hardcode84 created this revision.Apr 28 2021, 5:06 AM
Hardcode84 requested review of this revision.Apr 28 2021, 5:06 AM
rriddle added inline comments.Apr 28 2021, 10:18 AM
mlir/include/mlir/IR/PatternMatch.h
193

I would consider the name as part of the pattern, i.e. the name should be set in the constructor or via a set method by the pattern itself. There shouldn't be a need to expose this to any other class.

Hardcode84 added inline comments.Apr 28 2021, 10:31 AM
mlir/include/mlir/IR/PatternMatch.h
193

This makes sense from architectural POV but I'm not sure how to achieve this. And I definitely do not want to bother pattern writers with additional ctor parameter, needed only for debug purposes.

rriddle added inline comments.Apr 28 2021, 10:33 AM
mlir/include/mlir/IR/PatternMatch.h
193

I have the opposite POV. I'd rather do what's right architecturally. I don't have as much concern about pattern writers adding an additional param.

Hardcode84 added inline comments.Apr 28 2021, 11:01 AM
mlir/include/mlir/IR/PatternMatch.h
193

This is work which can and should be automated. Not to mention it will require update to all existing rewrites to be useful, which is unrealistic. Anyway, I agree that current solution is hacky and I don't have better at the moment. I guess I will just keep this as separate patch and will apply to my copy locally.

rriddle added inline comments.Apr 28 2021, 1:20 PM
mlir/include/mlir/IR/PatternMatch.h
193

I don't think it's completely hacky, and I do have a need/desire for this functionality as well. What if we restructured it such that:

  • A pattern may have multiple labels.
  • A public function on Pattern is exposed, e.g. addLabel(StringRef label)

At that point, you can move the need for any friends (which was the part that stuck out to me most).

I feel this is a bit "ad-hoc" and without more info here it may be a bit messy (collision, lack of naming convention, etc.) . It may not matter too much as a "debug name" use case but that could prevent other kind of use cases (filtering for example). A label system could be more flexible but "label" does not necessarily provide a unique id either. I'm not sure what'd be an ideal solution yet though.

Actually, maybe just rename those methods to get/setDebugName() or setPrettyName(), make them public and say explicitly in doc that it should be used only for debugging purposes? So 'proper' pattern name or tags can be added later separately if needed.

Actually, maybe just rename those methods to get/setDebugName() or setPrettyName(), make them public and say explicitly in doc that it should be used only for debugging purposes? So 'proper' pattern name or tags can be added later separately if needed.

SGTM. I can evolve it to fit my needs in a followup.

mlir/lib/Rewrite/PatternApplicator.cpp
206

Do we need to repeat the Trying to match here? Seems redundant?

Hardcode84 updated this revision to Diff 343353.May 6 2021, 4:08 AM

Renamed to setDebugName, moved to public, FnPattern handling

mlir/lib/Rewrite/PatternApplicator.cpp
206

removed

rriddle accepted this revision.May 6 2021, 11:22 AM
rriddle added inline comments.
mlir/include/mlir/IR/PatternMatch.h
995–998

?

mlir/lib/Rewrite/PatternApplicator.cpp
199–200

Is this useful? Is the result much worse if we just emit the empty name?

(Note that getPatternName is also going to need a (void)getPatternName to avoid unused variable errors in release mode)

This revision is now accepted and ready to land.May 6 2021, 11:22 AM
Hardcode84 added inline comments.May 6 2021, 11:32 AM
mlir/lib/Rewrite/PatternApplicator.cpp
199–200

This is debug output, so I don't have strong opinion on this. Empty pattern name will look like Trying to match "" (empty quotes)

Hardcode84 updated this revision to Diff 343627.May 7 2021, 3:13 AM

review comments

Do you need someone to land this for you?

mlir/lib/Rewrite/PatternApplicator.cpp
199–200

If it matters in-practice we can look at adding better defaults.