Page MenuHomePhabricator

Add indented raw_ostream class
ClosedPublic

Authored by jpienaar on Jul 18 2020, 12:27 PM.

Details

Summary

Class simplifies keeping track of the indentation while emitting. For every new line the current indentation is simply prefixed (if not at start of line, then it just emits as normal). Add a simple Region helper that makes it easy to have the C++ scope match the emitted scope.

Use this in op doc generator and rewrite generator.

Diff Detail

Event Timeline

jpienaar created this revision.Jul 18 2020, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2020, 12:27 PM

Any reason why you can't just use/improve llvm::ScopedPrinter instead? https://github.com/llvm/llvm-project/blob/2e0ee477127c44b86c018da7f24a622ec8f8fccd/llvm/include/llvm/Support/ScopedPrinter.h

Seems like this serves the exact same use case. Also, this seems like something that would go into llvm/Support and not mlir/Support.

Don't they serve different purposes? E.g., ScopedPrinter is meant to be used locally, it adds labels everywhere, can't be used where raw_ostream can and uses different calling conventions rather than << methods.

antiagainst accepted this revision.Aug 4 2020, 1:09 PM

This looks awesome! Sorry about the delay... LGTM; just a few nits. I see River has comments so would be good to get his consent too.

mlir/include/mlir/Support/IndentedOstream.h
40

Re-indents?

44

Increases?

50

Decreases?

65

Probably can delete this comment; not very useful.

70

currentPos?

78

s/uses/used/

mlir/tools/mlir-tblgen/RewriterGen.cpp
510

Are these tabs that will need clang-format?

This revision is now accepted and ready to land.Aug 4 2020, 1:09 PM
jpienaar updated this revision to Diff 295982.Oct 3 2020, 8:53 AM
jpienaar marked 6 inline comments as done.

Addressed typos and used more similar naming to ScopedPrinter

This revision was landed with ongoing or failed builds.Oct 3 2020, 8:54 AM
This revision was automatically updated to reflect the committed changes.

Isn't it something that belongs to the LLVM Support library?

Also, this seems like something that would go into llvm/Support and not mlir/Support.

Actually River commented on this but I don't think you addressed it?

Also, this seems like something that would go into llvm/Support and not mlir/Support.

That depends, almost everything in mlir/Support could also be in llvm/Support, as these are mostly general utils (e.g., nothing in StorageUniquer.h is MLIR unique it would seem). Wouldn't the question be is it used beyond MLIR too? E.g., just moving parts which have no current use in larger project to more core location doesn't make as much to me, moving it when it becomes used there seems more of a fit.

Actually River commented on this but I don't think you addressed it?

River asked if it should be an extension of ScopedPrinter which I did address.

mlir/include/mlir/Support/IndentedOstream.h
70

Overriding base class member, but yes inconsistent.

mlir/tools/mlir-tblgen/RewriterGen.cpp
510

Artifact of new phab interface (no tabs here)

Also, this seems like something that would go into llvm/Support and not mlir/Support.

That depends, almost everything in mlir/Support could also be in llvm/Support,

We have been moving everything we could, and if things are remaining from when MLIR was not in-tree, they are just "pending".

as these are mostly general utils (e.g., nothing in StorageUniquer.h is MLIR unique it would seem). Wouldn't the question be is it used beyond MLIR too? E.g., just moving parts which have no current use in larger project to more core location doesn't make as much to me, moving it when it becomes used there seems more of a fit.

I think this is backward: things won't get used unless they are in LLVM Support, and also this not in line with the existing practice either: we have been moving things from MLIR Support to LLVM support as much as possible.

Actually River commented on this but I don't think you addressed it?

River asked if it should be an extension of ScopedPrinter which I did address.

I quoted River above. His sentence was independent from the comment on ScopedPrinter: "Also, this seems like something that would go into llvm/Support and not mlir/Support."
I believe he was asking the same thing as I do.