parser graph generation support for graphs backend #969
Conversation
backends/graphs/controls.cpp
Outdated
| @@ -67,6 +67,18 @@ class EdgeSwitch : public EdgeTypeIface { | |||
| const IR::Expression *labelExpr; | |||
| }; | |||
|
|
|||
| class EdgeSelect : public EdgeTypeIface { | |||
There was a problem hiding this comment.
This is not used in this file, so I'm not sure why it is defined here
There was a problem hiding this comment.
This will be removed. Remnant code from my initial experimentation with code.
| @@ -14,8 +14,8 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| #ifndef _BACKENDS_GRAPHS_CONTROLS_H_ | |||
There was a problem hiding this comment.
it's a little confusing to call this file graphs.h, given that none of the classes it prototypes are implemented in graphs.cpp.
There was a problem hiding this comment.
It is the common header file for both control.cpp and parser.cpp.
If you have any other suggestion for names, i would appreciate that.
There was a problem hiding this comment.
Maybe graph_generators.h if you want to have a single header file for both control and parser
There was a problem hiding this comment.
If you refactor ParserGraphs and ControlGraphs into a single Graphs (abstract) base class with ParserGraphs and ControlGraphs as (simpler) subclasses, then I think this organization does make a fair amount of sense.
There was a problem hiding this comment.
I tried to refactor the classes and created a Graphs base class.
backends/graphs/graphs.h
Outdated
| // properties. See | ||
| // https://stackoverflow.com/questions/29312444/how-to-write-graphviz-subgraphs-with-boostwrite-graphviz | ||
| // for more information. | ||
| using GraphvizAttributes = std::map<cstring, cstring>; |
There was a problem hiding this comment.
it seems you pasted a lot of code from ControlGraphs here, and that these 2 classes would benefit from inheriting from a common base class
| bool preorder(const IR::P4Parser *pars) override; | ||
| bool preorder(const IR::ParserState *state) override; | ||
|
|
||
| void writeGraphToFile(const Graph &g, const cstring &name); |
There was a problem hiding this comment.
as underlined in my comment above, duplicating this method in ControlGraphs and ParserGraphs looks like a bad design
There was a problem hiding this comment.
Now the code duplication is minimized.
backends/graphs/parser.cpp
Outdated
| parents.clear(); | ||
|
|
||
| if (state->selectExpression->is<IR::PathExpression>()) { | ||
| parents = {{v, new EdgeSwitch(new IR::DefaultExpression())}}; |
There was a problem hiding this comment.
these edge classes are not visible from this file; this will break non-unified builds
There was a problem hiding this comment.
I don't understand what you said? I am pretty new to c++
Can you give little more details, plz. ?
There was a problem hiding this comment.
Well there is no EdgeSwitch declaration visible from this file. It is in controls.cpp. I believe that this is only compiling because we use a unified-build by default, which merges all the source files in this directory before compiling them. Some people build this code in a non-unified way, it which case it would fail. You should try compiling without a unified build (run cmake with UNIFIED_COMPILATION_ENABLED set to false). It really should be a regression for this repo IMO but it's not.
|
@sethfowler @cc10512 @ChrisDodd : there are several builds that fail due to some errors that should not happen, including this one. Is the Travis setup for testing wrong? |
|
@mbudiu-vmw agreed. It is likely the -march=native that breaks it when running in a VM. |
|
@mbudiu-vmw I updated the graphs to use rectangle. Please take a look at the example graph which is part of the commit and let me know your thoughts. |
backends/graphs/graphs.cpp
Outdated
| @@ -55,6 +55,116 @@ MidEnd::MidEnd(CompilerOptions& options) { | |||
| }); | |||
| } | |||
|
|
|||
| class Graphs::GraphAttributeSetter { | |||
There was a problem hiding this comment.
I personally think that the main function for the backend and the Graph methods implementation belong in 2 different files; apart from that it looks ok.
There was a problem hiding this comment.
created a new file "p4c-graphs.cpp" to accommodate main funcion.
|
Is there something else remain to be done from my side for this pull request to be merged? I would appreciate any input here to understand the pull/merge process. |
Generates a graph representation of top-level parser block as a dot file.
Added an example of parser graph for reference.