(Part 1) PSA support - addressing review comments #618
Conversation
There was a problem hiding this comment.
It is not easy to do a good review when one sees only the diff and all the previous comments are lost. I may have missed important things, but hopefully it won't be too bad.
What happened to the passes that manipulated the checksum? Have you checked that programs that use checksums still work? If you have one of these maybe we can add an STF test for it.
| const IR::Key* key, | ||
| Util::JsonObject* table, | ||
| Util::JsonArray* action_profiles) { | ||
| ControlConverter::handleTableImplementation(const IR::Property* implementation, |
There was a problem hiding this comment.
Indentation still wrong after you renamed the class.
backends/bmv2/extractArchInfo.cpp
Outdated
| @@ -47,10 +46,10 @@ bool InferArchitecture::preorder(const IR::Type_Control *node) { | |||
| return false; | |||
| } | |||
|
|
|||
| /// new Parser_Model object | |||
| bool InferArchitecture::preorder(const IR::Type_Parser *node) { | |||
| /// new P4::Parser_Model object | |||
There was a problem hiding this comment.
I don't understand this Doxygen comment, it is too short.
backends/bmv2/extractArchInfo.cpp
Outdated
| @@ -66,19 +65,19 @@ bool InferArchitecture::preorder(const IR::Type_Parser *node) { | |||
| /// p4type = p4Type(param); | |||
There was a problem hiding this comment.
This is a strange comment, usually one does not write pseudocode in comments.
backends/bmv2/extractArchInfo.cpp
Outdated
| for (auto p : node->getConstructorParameters()->parameters) { | ||
| LOG1("package constructor param: " << p); | ||
| auto ptype = typeMap->getType(p); | ||
| if (ptype == nullptr) | ||
| continue; | ||
| if (ptype->is<IR::P4Parser>()) { | ||
| LOG3("new parser model: " << p->toString()); | ||
| v2model.parsers.push_back(new Parser_Model(p->toString())); | ||
| v2model.parsers.push_back(new P4::Parser_Model(p->toString())); |
There was a problem hiding this comment.
It is dangerous to rely on the behavior of toString; maybe you can use instead p->name.
|
|
||
| class InferArchitecture : public Inspector { | ||
| class ExtractArchInfo : public Inspector { |
There was a problem hiding this comment.
Please document this class.
backends/bmv2/mapAnnotations.h
Outdated
| @@ -25,31 +25,18 @@ limitations under the License. | |||
| namespace BMV2 { | |||
|
|
|||
| /** | |||
| * Copy Annotations from architecture block to the corresponding | |||
| * instance in user program. | |||
| * Build a map from architecture block to its declaration in uer program. | |||
There was a problem hiding this comment.
I am hoping that this pass will not be necessary; annotations are a brittle way to do what I think you are doing.
The architecture description language is rich enough to expose all the information you need, you are building another language layer on top of it with annotations, which is not as precise.
|
I am going to work on the passes that deal with checksums today and add an STF test. |
hanw
force-pushed
the
hanw/psa2
branch
2 times, most recently
from
0134c1f to
97c49b6
Compare
- remove mapAnnotation pass in backend - reorg how --arch flags is handled in backend - move std::set<cstring> of control block name to backend.h
Addressed most of the comments from Mihai.
Need more refactor to get rid of P4V1 namespace. Will do in a later PR.