Page MenuHomePhabricator

Add a `printTrailingComment` method on OpAsmOpInterface
AbandonedPublic

Authored by mehdi_amini on Jan 21 2022, 8:51 PM.

Details

Summary

This is a demo for: https://llvm.discourse.group/t/adding-comments-to-declarative-assembly-format/5906/

It isn't fleshed out and requires more thoughts: again this is just a demo to help the discussion on Discourse.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 21 2022, 8:51 PM
mehdi_amini requested review of this revision.Jan 21 2022, 8:51 PM

Thanks!

mlir/lib/IR/AsmPrinter.cpp
2532–2535

braces not needed

stephenneuendorffer added inline comments.
mlir/test/lib/Dialect/Test/TestOps.td
666

This op prints....

This revision is now accepted and ready to land.Jan 21 2022, 9:19 PM
rriddle requested changes to this revision.Jan 21 2022, 9:23 PM

I'd rather not have trailing in the name. If we have multiple things adding comments, how is this going to interact?

This revision now requires changes to proceed.Jan 21 2022, 9:23 PM

I'd rather not have trailing in the name. If we have multiple things adding comments, how is this going to interact?

I used trailing to express that it comes after the op completes (after trailing locations), and also because nothing can come after a trailing comment on the same line.
We don't support /*.. */ comments anyway right?
I don't think anything else can add comments today, but if we added some in the future it shouldn't be an issue, you can accumulate trailing comments (// comment1 // comment2) :)

I'd rather not have trailing in the name. If we have multiple things adding comments, how is this going to interact?

you can accumulate trailing comments (// comment1 // comment2) :)

(assuming this is multiple comments on 1 line) -1 there, that looks clunky (especially if the comments are more than a few characters) and would likely be very unreadable.

That being said, I think right now your documentation leaves it open ended what "after the operation" means; i.e. I guess it could mean on separate line/same line/etc.
I would prefer we don't guarantee that the comment is on the same line as the operation, maybe we try to do that when we can (maybe one line and one comment?)?

mlir/include/mlir/IR/OpAsmInterface.td
65–71

Should this just be limited to the operation interface? Why not also add to the dialect interface? In some sense, I expect we may also end up allowing injection of comments from users outside of interfaces (though not important now).

66

Can you expand this a bit? Giving more detail, examples, etc?

It also isn't clear here if the comment is allowed to span multiple lines (which I guess not right now from the implementation?).

mlir/lib/IR/AsmPrinter.cpp
2532–2534

Should we always print comments?

mehdi_amini added inline comments.Jan 21 2022, 10:07 PM
mlir/include/mlir/IR/OpAsmInterface.td
65–71

When would the dialect interface be involved? If it is on a per-op basis, then the dialect can always implement the fallback for the op right?

mlir/lib/IR/AsmPrinter.cpp
2532–2534

You mean like: not printing them in the generic format for example?

mehdi_amini edited the summary of this revision. (Show Details)Jan 21 2022, 11:19 PM
mehdi_amini added inline comments.Feb 21 2022, 1:06 PM
mlir/include/mlir/IR/OpAsmInterface.td
65–71

Ping on this River?

Discussed this with River:

  • We want something more general: inject information to be printed as comment before, after, or trailing the operation.
  • The interface is restricting us to a single information to be printed per operation. We can have this "print user count" added as an external model and also add another one. We likely want something that composes better.
mehdi_amini abandoned this revision.Feb 22 2022, 10:18 PM