Add a compiler pass that adds missing IDs to control plane objects #3455
Conversation
fruffy
force-pushed
the
control_plane_pass
branch
3 times, most recently
from
3b84874
to
974e905
Compare
fruffy
force-pushed
the
control_plane_pass
branch
2 times, most recently
from
5818ed4
to
11a7e31
Compare
Include. Another build fix? Another build fix attempt. Cpplint and more include fixes. Edits. Fix compilation issue.
fruffy
force-pushed
the
control_plane_pass
branch
from
8ecdc4a
to
ad2a6bd
Compare
| @@ -0,0 +1,3 @@ | |||
| #include "addMissingIds.h" | |||
There was a problem hiding this comment.
you don't need to have a cpp file if it is empty.
| @@ -0,0 +1,400 @@ | |||
| /* | |||
There was a problem hiding this comment.
I will assume that definitions in this file are just moved from the other source.
There was a problem hiding this comment.
Yes, I only added a new function call.
| } | ||
| }); | ||
| } | ||
| void collectExternSymbols(P4RuntimeSymbolTable& symbols, |
There was a problem hiding this comment.
we usually leave an empty line between function definitions
control-plane/addMissingIds.h
Outdated
|
|
||
| namespace P4 { | ||
|
|
||
| /// Scans the P4 program, run the evaluator pass, and derives the P4Runtime Ids |
There was a problem hiding this comment.
Should this comment apply to the PassManager below?
| /// particular architecture. | ||
| const ControlPlaneAPI::P4RuntimeArchHandlerBuilderIface& archBuilder; | ||
|
|
||
| const IR::P4Program* preorder(IR::P4Program* program) override; |
There was a problem hiding this comment.
Here however empty lines are not useful.
control-plane/addMissingIds.cpp
Outdated
|
|
||
| passes.push_back(new ControlPlaneAPI::ParseP4RuntimeAnnotations()); | ||
| passes.push_back(new MissingIdAssigner(refMap, typeMap, *archBuilder)); | ||
| // Need to refresh types after we have updated some expressions with |
There was a problem hiding this comment.
the rule we use is the opposite: if a pass needs types, it should run first typeChecking.
So this should not be necessary - whatever comes after will do it if needed.
control-plane/addMissingIds.cpp
Outdated
| namespace P4 { | ||
|
|
||
| const IR::P4Program* MissingIdAssigner::preorder(IR::P4Program* program) { | ||
| auto evaluator = P4::EvaluatorPass(refMap, typeMap); |
There was a problem hiding this comment.
why hide the evaluator in this pass instead of running it from the PassManager?
There was a problem hiding this comment.
If I do not compute this in this specific order, the evaluator will return a nullptr for getTopLevelBlock. When I move the evaluator in the pass manager the reference to the top level block is not set yet.
It is a little awkward, the other way to solve this is to make the Symboltable mutable and move a reference to it around.
There was a problem hiding this comment.
I would like to not use the evaluator at all, the only reason I currently use it is to preserve the exact functionality as in p4RuntimeSerializer.cpp.
control-plane/addMissingIds.cpp
Outdated
| archBuilder(refMap, typeMap, toplevel)); | ||
| auto objectVector = IR::Vector<IR::Node>(); | ||
| for (const auto* obj : program->objects) { | ||
| objectVector.push_back(obj->apply( |
There was a problem hiding this comment.
this seems unnecessary - calling the visitor on the program should do exactly this.
You are also allocating a new visitor for each program object, that sounds wasteful.
There was a problem hiding this comment.
I was getting a segmentation fault for using *this. Let me try again.
control-plane/addMissingIds.cpp
Outdated
| } | ||
| size_t propertyIdx = 0; | ||
|
|
||
| const IR::Key* key = nullptr; |
There was a problem hiding this comment.
you are reconstructing here the logic of the visitor, why not just make a postorder function for a table property?
| return table; | ||
| } | ||
| auto* newKey = key->clone(); | ||
| IR::Vector<IR::KeyElement> newKeys; |
There was a problem hiding this comment.
same here, you can make this as a postorder function for a key
There was a problem hiding this comment.
I think I can't since I need to keep track of the individual elements to assign the appropriate IDs.
|
However, I would heed @antoninbas' comments carefully. I would really prefer if he approved the design. |
There was a problem hiding this comment.
This is fine by me as it seems to just be about re-organizing some code, and the P4RuntimeSerializer functionality is not impacted.
However, I don't understand the changes to backends/bmv2/. You are adding 2 include statements to backends/bmv2/simple_switch/midend.cpp, but no other code, so I am not sure why this change is for. I am not clear about the PR description either (Add a BMv2 pass that adds missing IDs to control plane objects). This change doesn't really seem related to bmv2.
fruffy
changed the title
|
This pass was first added to the Add this pass to other mid ends and removing the ID assignment portion in the serializer is a further TODO. |
This pull request moves the
SymbolTableand other objects out of theRuntimeSerializerso that it can be used by dedicated compiler passes. In this case, a new compiler pass (MissingIdAssigner), which uses the symbol table to attach IDs to control plane IR objects that are missing an ID. This pass can be used to transform a P4 program to have full P4Runtime information without needing to parse the P4Info file or serialized protobuf objects.These changes should not affect the normal control-plane behavior, but the computed IDs should be identical to the IDs computed by the serializer. To make the behavior identical, it is unfortunately necessary to use a dedicated evaluator pass within the ID assigner, which is redundant.
This pass can be used by any target back end right as first step after the front end. The only inputs needed are the ref and type map and the corresponding architecture handler.
Looking for some feedback on how to improve the structure of the pass. Because of the way the symbol table is constructed, it is necessary to compute it directly in the
IR::Programvisitor step, which is a little awkward. Also, I could try to refactor the entire serializer to expect IDs at all times and each back end has to run theMissingIdAssignerfirst, but this might be too invasive of a change.Fixes #3448.