This is an archive of the discontinued LLVM Phabricator instance.

Print custom assembly on pass failure by default
ClosedPublic

Authored by mehdi_amini on Apr 17 2022, 12:24 PM.

Details

Summary

The printer is now resilient to invalid IR and will already automatically
fallback to the generic form on invalid IR. Using the generic printer on
pass failure was a conservative option before the printer was made
failsafe.

Diff Detail

Event Timeline

mehdi_amini created this revision.Apr 17 2022, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2022, 12:24 PM
mehdi_amini requested review of this revision.Apr 17 2022, 12:24 PM

Improve testing

mlir/test/Pass/invalid-ir.mlir
3

If we want a flag to force the custom printer, it should address the two cases above.

jpienaar accepted this revision.Apr 17 2022, 3:01 PM

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

mlir/test/Pass/invalid-ir.mlir
2

invalid-ir-print-after-failure? (The file name sounds like it could be testing a verifier).

mlir/test/lib/Pass/TestPassManager.cpp
163

s/as well// ? (This could be triggered independently)

This revision is now accepted and ready to land.Apr 17 2022, 3:01 PM

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

lattner accepted this revision.Apr 18 2022, 11:07 AM

nice! Very excited this "just works" instead of being another knob for experts

rriddle accepted this revision.Apr 18 2022, 11:13 AM
rriddle added inline comments.
mlir/test/lib/Pass/TestPassManager.cpp
11–12

Are all of these new includes necessary?

142

This shouldn't need to be called explicitly, options are implicitly copied.

158

You can use the builder to create the unknown loc.

mehdi_amini marked 3 inline comments as done.

Address River's comments

mlir/test/lib/Pass/TestPassManager.cpp
11–12

I think we get them transitively and VSCode added it for me as I was using APIs.

mehdi_amini marked an inline comment as done.

Address Jacques' comment (rename file)

Will wait one more day to make sure @bondhugula and @stellaraccident can chime in as well.

bondhugula accepted this revision.Apr 19 2022, 12:45 AM

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

mlir/test/Pass/invalid-ir-print-after-failure.mlir
12 ↗(On Diff #423435)

Drop leading space.

mlir/test/Pass/ir-printing.mlir
7

Nit: You can just call this AFTER_FAILURE (since you have the other checks for valid/invalid in another file).

mlir/test/lib/Pass/TestPassManager.cpp
145

add -> adds

214

Nit: Sorted order here.

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

Your patch didn’t handle invalid IR, as I shown with the examples in one of my last answers there.

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

Without an extra flag, how do you get custom assembly when the IR is invalid for trivial reasons (like in the example I provided -- a subscript being an i32 type instead of an index type for example for a memref.load/affine.load in a nested loop)? There are several such cases in which custom printers will not crash.

bondhugula added a comment.EditedApr 19 2022, 1:00 AM

Nice, good point about the change to dump for invalid IR, this seems to provide the functionality of the previous change without needing a flag/API change and producing the prettier output in more cases. Of course the discussion still ongoing but looks like general improvement.

Not completely. It doesn't provide you with a custom assembly when the IR is invalid -- you *will* need an extra flag to say you need custom assembly even when the IR is invalid.

Your patch didn’t handle invalid IR, as I shown with the examples in one of my last answers there.

Right. (I went back and checked that.) We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

Why is this safe? Does the printer already check that the IR passes verify() and falls back to the generic form automatically?

If so, this is awesome. Does Operation::dump() have the same behavior?

Yes: this changed ~ last month I think.

That is what I was trying to explain in Uday's patch and why I was objecting to adding a flag.

Without an extra flag, how do you get custom assembly when the IR is invalid for trivial reasons (like in the example I provided -- a subscript being an i32 type instead of an index type for example for a memref.load/affine.load in a nested loop)? There are several such cases in which custom printers will not crash.

You need an extra flag, your patch didn’t handle this though, as I demonstrated with an example there.
This why I mentioned two separate cases: 1) for the pass failing but the ir valid, handled by your patch but subsumed here by changing the default, and 2) the invalid IR case which needs a flag, but neither this patch nor your patch handles

We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

No: I’ll land this as is and then see how the printer can be forced to assume the ir is verified

mehdi_amini marked 4 inline comments as done.Apr 19 2022, 10:28 AM
This revision was automatically updated to reflect the committed changes.

We need something more -- to force the printer to use the custom form at the risk of crashing. Are you planning to handle this in this revision?

No: I’ll land this as is and then see how the printer can be forced to assume the ir is verified

Seems like the existing set of flags is enough, I improved the test to demonstrate it here: https://github.com/llvm/llvm-project/commit/02eac667ed2dbf6eca4bc1195600eb0d3ecaff77

Let me know if anything is missing!

Thanks Mehdi (just catching up and LGTM).