Conversation
pearzt
left a comment
There was a problem hiding this comment.
Thanks a lot for all the work, the structure looks really good. I have some initial comments, but I will try to do a more in-depth review soon.
jplehr
left a comment
There was a problem hiding this comment.
Thank you for working on this.
I did an initial brief pass, please go through the comments and address them.
One more question: Does this PR as-is already run CI pipelines? If not, please add it to the CI pipelines as appropriate.
cgfcollector/src/Main.cpp
Outdated
| dot::DotGenerator dotGen(cg); | ||
| dotGen.generate(); | ||
|
|
||
| std::unique_ptr<llvm::raw_pwrite_stream> dotfile = ::createOutputFile(getInstance(), getCurrentFile(), "dot"); |
There was a problem hiding this comment.
This can be problematic in parallel CI runs
There was a problem hiding this comment.
I think we should separate the concerns between call graph generation and visualization. cgfcollector produces the call graph. If the user want a visualization, they can use your dot export tool for this (which is a nice addition, btw).
There was a problem hiding this comment.
That would indeed be nice: have the dot generator tool as a separate PR.
cgfcollector/src/Main.cpp
Outdated
|
|
||
| std::string cgString = dumpCG(); | ||
|
|
||
| std::unique_ptr<llvm::raw_pwrite_stream> file = ::createOutputFile(getInstance(), getCurrentFile(), ""); |
There was a problem hiding this comment.
This can be problematic in parallel CI runs
There was a problem hiding this comment.
Should this be named visual?
Is this limited to the Fortran support? I would assume this can be factored out into another PR and put in as a general tool.
Other people's thoughts?
There was a problem hiding this comment.
I agree that this is something that should extracted into a separate PR.
|
Please rebase this onto the most current |
… for debugging graph edges
… not inserted into cg
… instead of node ids
01f3d3f to
ed5d7ff
Compare
cgfcollector/src/Main.cpp
Outdated
| generateCG(getParsing().parseTree(), getCurrentFile()); | ||
|
|
||
| std::string cgString = dumpCG(); | ||
| std::unique_ptr<llvm::raw_pwrite_stream> file = ::createOutputFile(getInstance(), getCurrentFile(), "json"); |
There was a problem hiding this comment.
Please use the MCGWriter infrastructure for file output. For an example how to use this, have a look at tools/cgconvert/CGConvert.cpp.
cgfcollector/src/Main.cpp
Outdated
| dot::DotGenerator dotGen(cg); | ||
| dotGen.generate(); | ||
|
|
||
| std::unique_ptr<llvm::raw_pwrite_stream> dotfile = ::createOutputFile(getInstance(), getCurrentFile(), "dot"); |
There was a problem hiding this comment.
I think we should separate the concerns between call graph generation and visualization. cgfcollector produces the call graph. If the user want a visualization, they can use your dot export tool for this (which is a nice addition, btw).
|
|
||
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake DESTINATION lib/cmake) | ||
|
|
||
| # visuel program to generate dot file from graph |
There was a problem hiding this comment.
| # visuel program to generate dot file from graph | |
| # program to generate dot file from graph |
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake DESTINATION lib/cmake) | ||
|
|
||
| # visuel program to generate dot file from graph | ||
| add_executable(${PROJECT_NAME}-visuel ${CMAKE_CURRENT_SOURCE_DIR}/tools/visuel.cpp) |
There was a problem hiding this comment.
| add_executable(${PROJECT_NAME}-visuel ${CMAKE_CURRENT_SOURCE_DIR}/tools/visuel.cpp) | |
| add_executable(${PROJECT_NAME}-visual ${CMAKE_CURRENT_SOURCE_DIR}/tools/visuel.cpp) |
I think that's a typo
cgfcollector/CMakeLists.txt
Outdated
| set(INSTALL_OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/${OUTPUT_BASENAME}.install") | ||
|
|
||
| # build-time values for development | ||
| set(CGFCOLLECTOR_FILE_NAME "${CMAKE_CURRENT_BINARY_DIR}/lib${PROJECT_NAME}.so") |
There was a problem hiding this comment.
Should this say that it is a library?
CGFCOLLECTOR_FILE_NAME_SO or _LIB or so?
cgfcollector/CMakeLists.txt
Outdated
|
|
||
| # build-time values for development | ||
| set(CGFCOLLECTOR_FILE_NAME "${CMAKE_CURRENT_BINARY_DIR}/lib${PROJECT_NAME}.so") | ||
| set(CGFCOLLECTOR_CGDIFF "${CMAKE_BINARY_DIR}/tools/cgdiff/cgdiff") |
There was a problem hiding this comment.
Similarly should this be _BIN to indicate that's a binary?
| "cmake_base.txt" | ||
| "" | ||
| ) | ||
| file( |
cgfcollector/include/Edge.h
Outdated
|
|
||
| EdgeManager(std::vector<Edge>& edges) : edges(edges) {} | ||
|
|
||
| void addEdge(const EdgeSymbol& e, bool debug = true); |
There was a problem hiding this comment.
What does debug indicate here and should the default really be "true"?
If that is more a development thing, the default should be false and only set to true when in debug build or when set to verbose or so.
|
|
||
| #include <flang/Semantics/symbol.h> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
I think we should have this whole thing in a namespace.
I don't remember what we did with cgcollector2, but how about metacg::cgf or so?
| // visitor methods | ||
|
|
||
| template <typename A> | ||
| bool Pre(const A&) { |
There was a problem hiding this comment.
Why are all of those pre and post uppercase?
| // added to cg in postProcess step | ||
| std::vector<Edge> edges; | ||
|
|
||
| // all functions |
There was a problem hiding this comment.
This comment does not say anything
| // args when the AST walker is in the respective function. | ||
| std::vector<Function> currentFunctions; | ||
|
|
||
| // all types |
There was a problem hiding this comment.
This comment does not say anything
This adds a call graph collector for Fortran. The collector is implemented as a flang plugin and generates a call graph from source-level. See the tests for a list of language features this plugin covers.
This pull request is quite large, but it is not really possible splitting it in multiply requests without pushing broken code. The tests could be separate into a different pull request, but they are tightly coupled with the collector and would fail without it, so I think keeping it in one pull request makes sense.
What this adds:
cgfcollectorthat includes flang plugin, tests, tools, etc.cgfcollector_wrapper.shandcgfcollector_comp_wrapper.shthat simplify running the plugin.How to run: see
cgfcollector/README.mdThe commit history is a mess, and I don't know how strongly you value a clean git history and if I should clean this up.