Create explicit count calls for direct counters in P4_14->16 translation #421

ChrisDodd · 2017-04-03T05:07:26Z

This is a small piece of Pierce's code, just the part that creates explicit 'count' calls for direct counter externs.

mihaibudiu · 2017-04-03T15:32:00Z

We don't have a clear spec for direct counters in the P4-14 spec. I exepceted that they always count, i.e., you don't need to invoke the count method explicitly in each action. Also, it is not clear at all whether such counters are also meant to count misses (e.g., default_action execution).

The standard model (under construction) should specify the counter behavior clearly. Depending on the specification of the behavior of the counters, this code may need to be only inserted in the back-end, and not as P4-16 code. In fact, if the counter extern does not have a count method, then this code should not exist in the IR. Conversely, if the count method exists, we have to answer other important questions about it semantics, e.g., what happens if you call it multiple times in an action, or if you increment multiple counters in the same action.

mihaibudiu

I don't understand the point of this code.
It inserts operations which do not exist in the source and do not exist in the target code.

mihaibudiu · 2017-04-03T15:33:21Z

backends/bmv2/jsonconverter.cpp

+                } else if (em->originalExternType->name == v1model.directCounter.name) {
+                    if (em->method->name == v1model.directCounter.count.name) {
+                        BUG_CHECK(mc->arguments->size() == 0, "Expected 0 argument for %1%", mc);
+                        // Do not generate any code for this operation


I don't understand this. If you generate no code for the count, then what is the point of having it?

mihaibudiu · 2017-04-03T15:35:24Z

frontends/p4/fromv1.0/programStructure.cpp

@@ -626,7 +626,8 @@ void ProgramStructure::createDeparser() {

 const IR::P4Table*
 ProgramStructure::convertTable(const IR::V1Table* table, cstring newName,
-                               IR::IndexedVector<IR::Declaration>* stateful) {
+                               IR::IndexedVector<IR::Declaration>* stateful,
+                               std::map<cstring, cstring> &mapNames) {


I would give a more suggestive name to the mapNames argument, perhaps "actionRenameMap".

mihaibudiu · 2017-04-03T15:36:02Z

frontends/p4/fromv1.0/programStructure.cpp

@@ -1338,7 +1341,8 @@ const IR::Statement* ProgramStructure::convertPrimitive(const IR::Primitive* pri

 const IR::P4Action*
 ProgramStructure::convertAction(const IR::ActionFunction* action, cstring newName,
-                                const IR::Meter* meterToAccess) {
+                                const IR::Meter* meterToAccess,
+                                cstring counterToAccess) {


Why is there always a single counter to access?

I believe P4_14 only supports a single direct meter and a single direct counter on a table (though it is not really specified). This code only supports one direct meter and one direct counter per table.

I could not see the error message checking for this in the diff. Maybe it's somewhere else...

This is a problem with the existing code for both direct_counter and direct_meter that should be addressed.

mihaibudiu · 2017-04-03T15:36:25Z

frontends/p4/fromv1.0/programStructure.cpp

-    // Save the original action name in an annotation.
+    if (counterToAccess != nullptr) {
+        ExpressionConverter conv(this);
+        // add a writeback to the meter field


Is this comment wrong?

mihaibudiu · 2017-04-03T15:41:08Z

frontends/p4/fromv1.0/programStructure.cpp

+        auto mc = new IR::MethodCallExpression(Util::SourceInfo(), method,
+                                               emptyTypeArguments, args);
+        auto stat = new IR::MethodCallStatement(mc->srcInfo, mc);
+        body->push_back(stat);


If there is a return in the body this statement will be unreachable.

This was copied from the direct_meter code which is similarly incorrect.

P4_14 does not support return in action bodies, so this problem cannot occur.

mihaibudiu · 2017-04-03T15:52:27Z

frontends/p4/fromv1.0/programStructure.cpp

@@ -1565,8 +1599,15 @@ ProgramStructure::convertControl(const IR::V1Control* control, cstring newName)
                ::error("Cannot locate table %1%", c.first->table.name);
                return nullptr;
            }
-            if (std::find(usedTables.begin(), usedTables.end(), tbl) != usedTables.end())
-                directCounters.emplace(c.first->table.name, c.first->name);
+            if (std::find(usedTables.begin(), usedTables.end(), tbl) != usedTables.end()) {


What if a counter is used in multiple tables?
What if its count method is invoked multiple times?

ChrisDodd · 2017-04-03T16:38:53Z

That is the point of this change -- to make the behavior of the direct counters explicit. This is mostly for backends OTHER than bmv2-ss, as bmv2-ss has implicit direct counters (so we end up inserting the count calls, then just removing them again when we create the json).

mihaibudiu · 2017-04-03T16:40:55Z

The problem with this change is that you are allowing people to insert explicit calls to count, and then you ignore them.

If you plan to change bmv2 this makes sense, but otherwise for the v1model this is pointless.

mihaibudiu · 2017-04-03T16:49:33Z

If this is for the purpose of other back-ends this transformation should be done in the back-end, and not in the front-end.

mihaibudiu · 2017-04-03T17:01:32Z

To be clear, I actually like this approach of explicitly indicating counter operations better, it's just that this does not seem to be implemented correctly on the only target implementing v1model that comes with the public compiler: bmv2. So people will write code explicitly invoking count (or NOT invoking count), but bmv2 will do the wrong thing in these cases. So it would be nice if BMv2 would be changed to follow this model.

ChrisDodd · 2017-04-03T18:29:19Z

The plan is to switch from using simple_switch and v1model.p4 to using bmv2-psa and psa.p4 (portable switch arch, which is the planned p4_16 standard architecture). There will be a bunch more changes for that; this is just one small piece that seems relatively self-contained.

mihaibudiu · 2017-04-03T18:33:14Z

By definition P4-14 can only be executed on v1model.p4...
So the changes to the P4-14 front-end do not apply to psa.p4; the changes should not touch v1model.p4.

ChrisDodd · 2017-04-04T17:46:56Z

P4_14 can be executed on any architecture that can support it. v1model.p4 is just a stop-gap until we can have the 'proper' P4_16 stdarch -- once that is available, we want to get rid of v1model.p4 and make P4_14->16 translation target the p4_16 standard architecture.

mihaibudiu · 2017-04-04T17:50:25Z

This may be impossible, or it may restrict severely what can be in the standard architecture.
P4-14 has a lot of built-in assumptions about the architecture.
But I will let the architecture design committee sort that out.

…ion.

ChrisDodd requested a review from hanw April 3, 2017 05:07

mihaibudiu reviewed Apr 3, 2017

View reviewed changes

ChrisDodd force-pushed the cdodd branch from 7b951ec to d7d73f7 Compare April 4, 2017 15:36

ChrisDodd force-pushed the cdodd branch 3 times, most recently from 9b9175f to 498c040 Compare April 6, 2017 17:09

hanw approved these changes Apr 6, 2017

View reviewed changes

ChrisDodd force-pushed the cdodd branch 3 times, most recently from 6b0521b to baef6e4 Compare April 7, 2017 17:48

Create explicit count calls for direct counters in P4_14->16 translat…

07c408d

…ion.

ChrisDodd force-pushed the cdodd branch from baef6e4 to 07c408d Compare April 7, 2017 18:19

ChrisDodd merged commit 2c131b8 into p4lang:master Apr 7, 2017

mihaibudiu mentioned this pull request Apr 11, 2017

p4c-bm2-ss gives incorrect warning about unused counter #461

Closed

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create explicit count calls for direct counters in P4_14->16 translation #421

Create explicit count calls for direct counters in P4_14->16 translation #421

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu left a comment

mihaibudiu Apr 3, 2017

mihaibudiu Apr 3, 2017

mihaibudiu Apr 3, 2017

ChrisDodd Apr 3, 2017

mihaibudiu Apr 3, 2017

ChrisDodd Apr 3, 2017

mihaibudiu Apr 3, 2017

mihaibudiu Apr 3, 2017

ChrisDodd Apr 3, 2017

ChrisDodd Apr 7, 2017

mihaibudiu Apr 3, 2017

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

ChrisDodd commented Apr 4, 2017

mihaibudiu commented Apr 4, 2017

Create explicit count calls for direct counters in P4_14->16 translation #421

Create explicit count calls for direct counters in P4_14->16 translation #421

Conversation

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

ChrisDodd commented Apr 3, 2017

mihaibudiu commented Apr 3, 2017

ChrisDodd commented Apr 4, 2017

mihaibudiu commented Apr 4, 2017