Rename the legacy DOTGraphTraits{Module,}{Viewer,Printer} to the corresponding DOTGraphTraits...WrapperPass, and implement a new DOTGraphTraitsViewer with new pass manager.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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. |
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 😄 .
Add runViewerImpl and runPrinterImpl to store the common implementation of the new and legacy pass.
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. |
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. |
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. |
Use pointer specialization by default, e.g. GraphTraits<ScopDetection *> but not GraphTraits<ScopDetection>
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".