Page MenuHomePhabricator

[mlir] Support drawing control-flow graphs in ViewOpGraph.cpp
ClosedPublic

Authored by springerm on Jul 19 2021, 9:31 PM.

Details

Summary
  • Add new pass option print-data-flow-edges, default value true.
  • Add new pass option print-control-flow-edges, default value false.

Depends On D106337

Diff Detail

Event Timeline

springerm created this revision.Jul 19 2021, 9:31 PM
springerm requested review of this revision.Jul 19 2021, 9:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 9:31 PM
rriddle added inline comments.Jul 22 2021, 10:22 AM
mlir/lib/Transforms/ViewOpGraph.cpp
162–163

Can we change these std::string params to Twine/StringRef?

244–251
245
247

Is the -> here necessary?

248

Please add a comment for constant params with the name of the parameter

272

Same here, why the ->?

276

Do you need to convert to std::string here? Or can you use Twine?

Thanks for the patch!

Is the intention to remove the need for ViewRegionGraph?

springerm marked 5 inline comments as done.Jul 22 2021, 11:03 PM
springerm added inline comments.
mlir/lib/Transforms/ViewOpGraph.cpp
162–163

Twines are used to avoid constructing temporary std::string, right? If so, AttributeMap should also be changed from StringMap<std::string> to StringMap<Twine>, but that's not possible because Twine's operator= is deleted.

Also, the label will immediately be passed into escapeString, which requires converting it into an std::string again. With this in mind, is there actually any benefit of using Twine here?

276

The question is whether emitEdgeStmt should take an std::string or a Twine. See comment above, will change all callers of emitEdgeStmt accordingly.

address comments

springerm added a comment.EditedJul 22 2021, 11:08 PM

Thanks for the patch!

Is the intention to remove the need for ViewRegionGraph?

I didn't know that we have ViewRegionGraph. I think this commit would make it obsolete, as it seems to do the same thing (drawing a CFG).

Can you provide an example of what the output here looks like compared to ViewRegionGraph? If it is close enough, I'd just delete ViewRegionGraph.

Can you provide an example of what the output here looks like compared to ViewRegionGraph? If it is close enough, I'd just delete ViewRegionGraph.

The output of ViewRegionGraph doesn't seem very useful....

rriddle accepted this revision.Jul 23 2021, 10:59 PM

The new output looks much better IMO. As a final test (you don't have to upload this one), can you check that it produces something reasonable on a larger function? says thousands/low tens of thousands of ops? I've produced CFG dot graphs in LLVM for functions like that, and they get quite large. If the output is even semi reasonable at that scale, we should delete ViewRegionGraph.

mlir/include/mlir/Transforms/Passes.td
688–699

This description likely needs an update.

706–709

It isn't obvious to me that control flow edges should be off by default, I'd say that is more a matter of which dialect you are in. What is the downside to having it always enabled?

mlir/lib/Transforms/ViewOpGraph.cpp
162–163

If it is being converted to std::string anyways, that is fine. The one nice benefit of Twine though, is that it results in a single std::string creation instead of potentially multiple (depending on what the user does).

(Also, I didn't read the code that far, seeing std::string in an LLVM code base is just a code smell)

This revision is now accepted and ready to land.Jul 23 2021, 10:59 PM
springerm marked an inline comment as done.Jul 24 2021, 4:39 AM
springerm added inline comments.
mlir/include/mlir/Transforms/Passes.td
706–709

I had them enabled by default, but some guys from a hardware team said that the CFG edges were confusing to them. They are working with TF graphs (or some lowering thereof), so they didn't really need them.

I have also seen cases where a constant is defined at the beginning of a function and then used further down. Having both data flow and control flow edges at the same time lead to a quite unreadable rendering with very long edges.

Which one you want (control/data flow) depends on the dialect I guess... But I think the default should be either data flow or control flow.

springerm updated this revision to Diff 361437.Jul 24 2021, 4:41 AM

minor update

This revision was landed with ongoing or failed builds.Aug 4 2021, 4:45 AM
This revision was automatically updated to reflect the committed changes.