This is an archive of the discontinued LLVM Phabricator instance.

[PassManager] Implement DOTGraphTraitsViewer under NPM
ClosedPublic

Authored by YangKeao on Apr 13 2022, 6:21 AM.

Details

Summary

Rename the legacy DOTGraphTraits{Module,}{Viewer,Printer} to the corresponding DOTGraphTraits...WrapperPass, and implement a new DOTGraphTraitsViewer with new pass manager.

Diff Detail

Event Timeline

YangKeao created this revision.Apr 13 2022, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 6:21 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
YangKeao requested review of this revision.Apr 13 2022, 6:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 6:21 AM
myhsu added a subscriber: myhsu.Apr 13 2022, 5:40 PM

I understand that most of the code was copied from the original (LegacyPM) implementation, but I feel like it's also a good time to fix some of the minor issues as highlighted by my inline comments.

llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
77

could we use struct and remove this?

91

the "llvm::" is redundant since we're already enclosed in namespace llvm

104

Please use Twine here. Also, F.getName() is sufficient, no need to create another std::string (via str())

116

Is there any specific reason we want to own the string? Is it possible to replace it with StringRef?

YangKeao updated this revision to Diff 422782.Apr 14 2022, 2:34 AM

Fix according to comments

YangKeao marked 4 inline comments as done.Apr 14 2022, 2:35 AM

The usual scheme for legacy pass names is usually to append either WrapperPass (such as DominatorTreeWrapperPass) or LegacyPass (such as CFGPrinterLegacyPass). Could you use one of those conventions?

Could you upload the patch with context (git diff -U999999, see https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

DOTGraphTraitsPrinter is not used in this patch. Do you plan to also introduce NPM passes for DomPrinter etc that derive from DOTGraphTraitsViewer?

llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
92–112

The function is now duplicated with LegacyDOTGraphTraitsPrinter::runOnFunction. You could either move it out into a common function, are call this NPM run function from the legacy passes' runOnFunction. See e.g. InstSimplifyLegacyPass::runOnFunction how this was was done.

YangKeao updated this revision to Diff 423599.Apr 19 2022, 5:48 AM
  1. Modify the name from Legacy* to *LegacyPass
  2. Upload the diff with git diff -U999999
YangKeao added a comment.EditedApr 19 2022, 5:50 AM

@Meinersbur

DOTGraphTraitsPrinter is not used in this patch.

It is actually used in the D123678 , by ScopPrinter

Do you plan to also introduce NPM passes for DomPrinter etc that derive from DOTGraphTraitsViewer?

I'll try 😄 .

YangKeao updated this revision to Diff 423603.Apr 19 2022, 6:15 AM

Add runViewerImpl and runPrinterImpl to store the common implementation of the new and legacy pass.

YangKeao edited the summary of this revision. (Show Details)Apr 19 2022, 6:15 AM
YangKeao marked an inline comment as done.
YangKeao updated this revision to Diff 423791.Apr 19 2022, 7:40 PM
myhsu added a comment.Apr 19 2022, 9:09 PM

Thanks for the patch, lgtm for most of the parts. Please address my comment though (especially the static one)

llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
32

I think static is redundant in this case.
Also, in LLVM, "XXXImpl" sounds more like a name for internal / implementation-specific code, but this function is global. I would recommend changing this (and runPrinterImpl) into something more general, like "viewGraphForFunction".

YangKeao updated this revision to Diff 423816.Apr 19 2022, 10:46 PM

Modify name from run*Impl to *GraphForFunction

YangKeao marked an inline comment as done.Apr 19 2022, 10:46 PM

DOTGraphTraitsPrinter is not used in this patch.

It is actually used in the D123678 , by ScopPrinter

Usually patches organized self-contained units of changes, which includes not introducing dead code. If you need the code only in D123678, introduce the code there. There is a possibility to have a refactoring/NFC patch to make the diff of a followup patch more readable, but those also do not introduce dead code.

A possibility to not introduce dead code would be to use the code in NPM versions of DomPrinter etc, which is why I was suggesting it.

llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
32

I don't see static being redundant here, it changes it to internal linkage. A template function is implicitly inline. Without static, every TU including this file will have to emit it, influencing compilation time.

Meinersbur accepted this revision.Apr 20 2022, 1:11 PM

No need to reorganize your patches now, but please keep it in mind for your next contributions.

LG too commit, or update it to introduce NPM passes.

llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
32

This actually isn't a change request, just wanted to point out that static has an effect. Inline non-static allows the linker to select just one of the definitions whereas static inline there might be multiple identical definitions the the final artifact. No opinion on which is better.

This revision is now accepted and ready to land.Apr 20 2022, 1:11 PM
YangKeao added a comment.EditedApr 21 2022, 5:03 AM

@Meinersbur Could you help me to commit this? 🍻

@Meinersbur Could you help me to commit this? 🍻

Sure, I can commit this together with D123678 for having a use of DOTGraphTraitsPrinter. Seems you still want to make a change to D123678?

YangKeao updated this revision to Diff 426663.May 3 2022, 5:30 AM

Fix wrong Twine usage

YangKeao updated this revision to Diff 426665.May 3 2022, 5:35 AM
YangKeao updated this revision to Diff 426911.May 3 2022, 9:12 PM

format codes

Fix wrong Twine usage

Right. I should have noticed this too.

YangKeao updated this revision to Diff 427674.May 6 2022, 10:30 AM

Use pointer specialization by default, e.g. GraphTraits<ScopDetection *> but not GraphTraits<ScopDetection>

Meinersbur accepted this revision.May 6 2022, 11:42 AM
This revision was landed with ongoing or failed builds.May 9 2022, 12:06 PM
This revision was automatically updated to reflect the committed changes.