Page MenuHomePhabricator

[mlir] Improve Graphviz visualization in PrintOpPass
ClosedPublic

Authored by springerm on Jul 18 2021, 11:50 PM.

Details

Summary
  • Visualize blocks and regions as subgraphs.
  • Generate DOT file directly instead of using GraphTraits. GraphTraits does not support subgraphs, colors, etc.

Diff Detail

Event Timeline

springerm created this revision.Jul 18 2021, 11:50 PM
springerm requested review of this revision.Jul 18 2021, 11:50 PM
rriddle requested changes to this revision.Jul 18 2021, 11:55 PM
rriddle added inline comments.
mlir/include/mlir/Transforms/Passes.td
703–704

Can you split these features into separate commits? Some of them have less obvious value than others. It also makes the review easier.

mlir/lib/Transforms/ViewOpGraph.cpp
18

Namespaces should only really contain classes. Please move everything else out.

93–94

Please use proper C++ constructor field initialization.

262

The llvm:: here shouldn't be necessary.

This revision now requires changes to proceed.Jul 18 2021, 11:55 PM
springerm updated this revision to Diff 359747.Jul 19 2021, 4:40 AM
springerm marked an inline comment as done.

address comments

springerm marked 3 inline comments as done.Jul 19 2021, 4:41 AM
springerm updated this revision to Diff 359748.Jul 19 2021, 4:43 AM

remove unnecessary line

For reference:

springerm updated this revision to Diff 359752.Jul 19 2021, 5:02 AM

simplify code

jpienaar added inline comments.
mlir/lib/Transforms/ViewOpGraph.cpp
19

I'd rather have this be kShapeNode which documents it's use rather than have kX == lowercase(X), else seems like a magical constant with extra step.

66

There can be multiple visualizations of an MLIR module (dominated by, will execute after, CDG, ...) it would be good to spell this out what this particular one is visualizing (which could refer to the description of pass for more info if appropriate)

83

We have an indented ostream, why not use that?

180

We have operations with very large number of results, always adding them to label will make this unusable there.

194

When I did one of these recently I made the arguments a table, that way arguments were just named numerically and associated with block entry. It kept all the arguments together and avoided it appearing the same as ops.

springerm marked an inline comment as done.Jul 19 2021, 6:56 PM
springerm added inline comments.
mlir/lib/Transforms/ViewOpGraph.cpp
66

Done. This is a dataflow visualization.

Btw, I'm planning to add a CFG visualization (to be enabled via a pass option) in a subsequent commit.

83

Yes, raw_indented_ostream is what we need here. But I couldn't find a way to use it in a pass.

The problem is that:

  • raw_indented_ostream's copy constructor is deleted.
  • The generated clonePass function in mlir/Transforms/Passes.h.inc calls the copy constructor of the pass.
  • Therefore, the raw_indented_ostream would have to be passed in as a reference. But there's no other suitable place to create a raw_indented_ostream (other than the constructor of the pass).
  • Alternative: Change tools/mlir-tblgen/PassGen.cpp and add a tablegen option to avoid emitting clonePass functions.
180

I'd add this in a separate commit if that's OK. (To keep this one small.) To be enabled via a pass option.

194

Not sure what you mean by that. Are you referring to Graphviz HTML tables? (https://graphviz.org/doc/info/shapes.html#html) With Graphviz tables I could generate a single node for all block arguments of a block. Then draw arrows from each cell to their respective uses.

Or do you recommend not generating nodes for block arguments at all?

Btw, in some of my experiments I found it useful to duplicate block argument nodes and constant nodes (e.g. %c0). Such nodes are often used many times throughout the IR, which renders quite unreadable due to the many edges. Also, those edges are quite long. What helps is creating a new block arg/constant node for each use, which the Graphviz engine will render close to the user, resulting in shorter edges (and less lines on the screen). I'll add this functionality in a subsequent commit.

springerm updated this revision to Diff 359984.Jul 19 2021, 6:57 PM

address comments

springerm updated this revision to Diff 360007.Jul 19 2021, 9:30 PM

minor improvements

For reference:

The floating 0 below test.br is weird, could that be avoided?

mlir/lib/Transforms/ViewOpGraph.cpp
83

Yes, raw_ostream's copy constructor is deleted, which results in raw_indented_ostream's being deleted. If you just added a copy constructor for this pass where you create a new one, that should work for this case (what would happen I believe is a it would be created with underlying raw_ostream, so you'd lose the indendation of the passed in indented ostream [which is fine as there is no expectation that the cloned pass and the original share indentation]).

113

Why not combine these? Use interleaveComma on the map & in functor create the string rather than creating all the strings and then outputting.

194

Yes, an HTML table. But indeed I ran into the same issue with long lines on larger graphs. Duplication is good idea for keeping it scoped - it makes it more difficult to notice when 2 ops have the same input (so if one has patterns that match equal operands then that is more desirable).

springerm marked 2 inline comments as done.Jul 22 2021, 11:02 PM

For reference:

The floating 0 below test.br is weird, could that be avoided?

This is a rendering issue. Edges have labels to indicate that a value is used as the 0th, 1st, 2nd, etc. operand. Those edges are clipped at cluster (subgraph) boundaries, but for some reason the labels are not. I think this is a bug in Graphviz. As a workaround, I stopped printing labels for edges that start/end at a cluster boundary.

address comments

springerm updated this revision to Diff 361435.Jul 24 2021, 4:39 AM

minor update

Are there any other changes that I should make to this revision?

In case you are worried about the std::string, which are considered a code smell as you said: There is a way to avoid a few of the std::string. By writing writing Graphviz attributes directly onto the stream instead of storing them in an AttributeMap. But that would reduce code readability and some level of abstraction.

Basically, I could no longer use helper functions such as emitAttrList. These would have to be duplicated wherever attributes are printed. Also more lambdas are likely needed for printing labels. The main reason why I added emitAttrList and AttributeMap is that it gives the code some more structure. Note that parts of the function names are derived from the DOT language specification. E.g., the language spec defines things such as attr_list. I tried to mirror that structure in the code as good as possible.

For reference: DOT language spec is here: https://graphviz.org/doc/info/lang.html

rriddle accepted this revision.Aug 3 2021, 6:01 PM

Sorry, heavily in and out of OOO recently.

Took a glance, looks good to land after resolving comments. Thanks!

mlir/include/mlir/Transforms/Passes.td
690–691

Unrelated, but I don't think this should be a ModuleOp pass. This feels like it should be a general "any op" pass.

mlir/lib/Transforms/ViewOpGraph.cpp
49–50

Can you move this to the description field of the pass?

59–61
64

Optional is exported in the mlir namespace, so the llvm:: here can be dropped.

85
195
203
257
263

Where is plainOs used? It looks like it can be dropped.

This revision is now accepted and ready to land.Aug 3 2021, 6:01 PM
springerm marked 7 inline comments as done.Aug 3 2021, 6:58 PM

Thanks for the review!

mlir/lib/Transforms/ViewOpGraph.cpp
263

This is necessary to implement the copy constructor (required by clonePass) correctly. There's no way to copy the indented output stream, so I have to construct a new one. That's where I pass in plainOs.

See my response to jpienaar's comment regarding the indented output stream for more details.

springerm updated this revision to Diff 363934.Aug 3 2021, 7:00 PM

address comments

rriddle added inline comments.Aug 3 2021, 7:06 PM
mlir/lib/Transforms/ViewOpGraph.cpp
263

Can we just add a raw_ostream &getOStream() { return os; } method to raw_indented_ostream then? The use of plainOs feels a bit hacky.

springerm updated this revision to Diff 363938.Aug 3 2021, 7:12 PM

no change

springerm updated this revision to Diff 363941.Aug 3 2021, 7:24 PM
springerm marked an inline comment as done.

address comments

springerm edited the summary of this revision. (Show Details)Aug 3 2021, 7:25 PM
This revision was landed with ongoing or failed builds.Aug 3 2021, 7:57 PM
This revision was automatically updated to reflect the committed changes.