Page MenuHomePhabricator

[MLIR] Improve doc for -mlir-print-local-scope and unhide
ClosedPublic

Authored by bondhugula on Jan 12 2022, 11:38 PM.

Details

Summary

This is a pretty important debugging option to stay hidden. Also,
improve its cmd-line description; the current description gives no hint
that this is the one to use to have locations printed inline.
Out-of-line locations are also unproductive to work with in many cases
where the locations are actually compact, which is also why this option
should be more visible. This revision doesn't change the default on it
though.

Diff Detail

Event Timeline

bondhugula created this revision.Jan 12 2022, 11:38 PM
bondhugula requested review of this revision.Jan 12 2022, 11:38 PM

Ideally, if this option shouldn't be hidden, inline location printing should get its own flag that is visible -- just like -mlir-print-debuginfo is visible?

bondhugula edited the summary of this revision. (Show Details)Jan 12 2022, 11:39 PM
bondhugula added a reviewer: jpienaar.
rriddle accepted this revision.Jan 12 2022, 11:43 PM

Ideally, if this option shouldn't be hidden, inline location printing should get its own flag that is visible -- just like -mlir-print-debuginfo is visible?

Inline location printing is related to if aliases are enabled or not, though it's likely not immediately obvious that those are just attributes. What would such a flag provide over print-local-scope?

mlir/lib/IR/AsmPrinter.cpp
147

Can you tweak this to mention that attribute/type aliases are disabled? (which is why locations are printed inline).

This revision is now accepted and ready to land.Jan 12 2022, 11:43 PM

Ideally, if this option shouldn't be hidden, inline location printing should get its own flag that is visible -- just like -mlir-print-debuginfo is visible?

Inline location printing is related to if aliases are enabled or not, though it's likely not immediately obvious that those are just attributes. What would such a flag provide over print-local-scope?

  1. You'll be able to keep print-local-scope hidden like before (if that was the right thing) while unhiding the other flag.
  2. print-local-scope gives no inkling that it would enable inline location printing (a search of mlir-opt -h or -help-hidden wouldn't help). Although I already knew there was some flag that allowed printing locations inline, it took me 15 min to find that again. I remember digging it from the commit message about 3 months ago but this isn't easy to remember. (A hidden flag makes it worse.) Having "location" in the help message or in the flag will definitely help.
bondhugula added inline comments.Jan 13 2022, 2:23 AM
mlir/lib/IR/AsmPrinter.cpp
147

This would be too much information for a help message I think. Any suggestions on the phrasing?

jpienaar accepted this revision.Jan 13 2022, 7:50 PM
jpienaar added inline comments.
mlir/lib/IR/AsmPrinter.cpp
147

"Print with all info inline (elides aliases)" ? Info is a bit broad but loc/attr/type may be too long. Alternatively perhaps "Print with all aliases expanded and elides" may not be much better.

I agree that local scope assumes more knowledge of internals than is strictly needed.

bondhugula added inline comments.Jan 13 2022, 8:35 PM
mlir/lib/IR/AsmPrinter.cpp
147

Thanks, @jpienaar -- I think these make it more understandable. Slightly longer is fine. Does "loc/attr/type" cover it all? There is local SSA numbering I think that's in fact a prominent part. Otherwise, this would be useful to users:

Print with inline information (eliding aliases for attributes, types, and locations)

Tweak message.

bondhugula marked 2 inline comments as done.Jan 18 2022, 5:17 PM