Page MenuHomePhabricator

[mlir] Print a comment when printing an op that failed to verify
Needs RevisionPublic

Authored by Mogball on Aug 30 2022, 11:49 AM.

Details

Summary

This patch changes the printer to print a comment when an op fails to
verify.

func @foo() {
  // Verification failed, printing generic form
  "test.invalid_op"() : () -> ()
  return
}

The verification of the op is done lazily instead.

Fixes #57435

Diff Detail

Event Timeline

Mogball created this revision.Aug 30 2022, 11:49 AM
Mogball requested review of this revision.Aug 30 2022, 11:49 AM
rriddle requested changes to this revision.Aug 30 2022, 11:53 AM
rriddle added inline comments.
mlir/lib/IR/AsmPrinter.cpp
2783–2810

This is going to run on every single operation (which in turn will recursively verify), we should not be doing that. AsmState is still the right place to do verification.

This revision now requires changes to proceed.Aug 30 2022, 11:53 AM

I would also rather have the comment at the top of the IR dump anyways, otherwise it's going to be difficult to spot if the dump is non-trivial.

Mogball added inline comments.Aug 30 2022, 1:19 PM
mlir/lib/IR/AsmPrinter.cpp
2783–2810

Right. But I'm struggling to find a place to print the comment otherwise. It can't be printed until an OperationPrinter is instantiated. I can pass a flag from AsmState to OperationPrinter and print the comment whenever the latter is instantiated but that isn't very pretty :( Any suggestions?

Awesome, thank you for improving this!

rriddle added inline comments.Aug 30 2022, 1:37 PM
mlir/lib/IR/AsmPrinter.cpp
2783–2810

Passing a flag seems like the best thing to me IMO (which doesn't need to be publicly exposed, you could expose whatever is necessary just in AsmStateImpl). The reasoning is that we need the flags to be correct in AsmState, because that directly impacts how values are numbered, aliases are initialized, etc. AsmState is also potentially used to separately print operations, and if we don't have that be the source of truth for flags, we'll end up with inconsistent results when printing different operations that use the same AsmState.