Turn transformations that happen within the BMV2 JSON converter into passes

[sethfowler]

During the process of BMV2 JSON generation, the JSON converter applies a number of transformations to IR. A few examples:

• _valid fields are added to header types. (This is used to implement isValid()).
• A .drop action is synthesized.
• Exit tables are synthesized for ingress and egress control blocks.
• The standard metadata struct type is effectively transformed into a header type, complete with a _valid member. (It may be that this is best handled totally outside of the compiler, at the P4 level.)

I think it would be a good idea to convert as much of this stuff as possible into compiler passes. This would render these transformations reusable and simplify the BMV2 JSON converter. Additionally, the PI generator currently needs to perform exactly the same transformations, which both increases its complexity greatly and prevents it from being used with backends other than BMV2. It'd be ideal to make the PI generator generic, and we'd be a lot closer to doing that if we convert these transformations into passes.

[mihaibudiu]

The pass is indeed rather complicated. It has grown "organically".

The bmv2 back-end does have a series of small lowering passes which attempt to make all changes that can be made at the IR level. There is also the ExpressionConverter pass which is invoked to convert expressions into JSON.

I don't think that the .drop action and the exit tables are used; they were part of an attempt to implement the semantics of P4-14 drop more cleanly, which failed. The main problem was that (at that time) the PI was not using the default_action setting to initialize tables at start-up. So drop is still being implemented today as a custom extern function.

The standard metadata (and in fact all metadata) is converted to a header_type because this is the only type of struct in P4-14.

I don't see any _valid fields being added anywhere. isValid is compiled according to the JSON spec here: https://github.com/p4lang/behavioral-model/blob/master/docs/JSON_format.md into accesses to $valid$.

One reason the code is so convoluted is that it has to remap P4-16 structures to P4-14 representations, and in particular P4-14 primitives, which sometimes have very different scoping rules. The p4c-bm2-ss back-end is seen as a temporary compatibility-providing solution.

In the long term the hope is that the standard architecture should be implemented in BMv2 and it should make use of externs rather than many of the built-in P4-14 primitives. Then the JSON generation should be substantially simpler.

[sethfowler]

Ah yes, you're right about _valid. That's actually added on the PI end; it's implicit for all header types. (Which is why it also happens for standard metadata.)

It sounds like some of this stuff can ultimately just be removed, which is the best-case scenario. =) Should I write up a PR to remove the ".drop" action and the exit table stuff? That doesn't seem too hard, and it may be better to do that sooner rather than later, so nothing starts depending on them.

Would you object to just defining the standard metadata struct as a header? That would be a quick simplification win and it seems like nothing that consumes BMV2 JSON can possibly be depending on it being a struct, since we convert it to a header in the JSON. On the other hand, I'm not sure if this would break any P4 programs that we need to maintain backwards compatibility with. It seems like structs support a strict subset of the operations which are acceptable on headers, so superficially it seems like it should be OK, but I'm not sure.

[mihaibudiu]

I hope that there are no dependences on the .drop action and the exit tables, so it should not be too difficult to remove them.

The standard_metadata is actually defined in v1model.p4 and it is intended to model the P4-14 standard metadata. Logically it is a struct: it has no validity bit, and you cannot pass it to extract methods as an argument; even in P4-14 it is a header_type instantiated as metadata. I don't really see the benefit of making it a header. I would think that treating standard_metadata is a header is rather a bug in the PI generation code.

Unfortunately these fixes will do very little so simplify this code...

What is the end goal? It depends whether you want to maintain this code, add new functionality (e.g., PI generation), or write the new standard-architecture back-end.

I believe that the standard architecture back-end should be rewritten from scratch, possibly stealing some code from this back-end.

[sethfowler]

The motivation here is that I've already written a big patch to add PI generation (a PR will be incoming soon) and I'm trying to figure out how to make the PI generation code "dumber" so it's not closely tied to the BMV2 backend. That's where the main simplification lies: in making the PI generation code as generic as possible. Removing some of this logic from the BMV2 JSON generator is just a side benefit, and admittedly not a huge one.

Thanks for your feedback about these ideas. One constraint I've been operating under has been to try to match the results of the existing "BMV2 JSON -> PI" pipeline, but it's becoming increasingly clear to me that some of the oddities that exist in that pipeline are a result of the fact that we don't generate the PI directly. I think at this point the best thing for me to do is to merge my code (which is compatible with the existing pipeline) and then start improving both my code and the PI code so that we can clean some of this stuff up. Things should gradually get simpler.

It does sound like the ".drop" action and the exit tables can be removed right away so I may go ahead and attempt that now.

I'll leave this issue open for now as a reminder that this should get done.

[antoninbas]

I think that the observation that the "BMv2 JSON -> PI" pipeline is a limited one is pretty important here. While this pipeline works well enough for P4_14, it doesn't for P4_16 -see our discussion about using expressions as match keys.
For the specific issue of the standard metadata, would it help if the PI BMv2 JSON reader did not add the _valid field for metadata header types?

[sethfowler]

It would help, yeah - things would be cleaner. To make things truly uniform, I think Mihai's right that we should just expose structs to PI. There would be no need to special-case metadata at all in that case. It would change how header instance fields are named, though. Let's say we have this setup:

struct standard_metadata {
    bit<9>  port;
    ...
}

header ipv4 {
    bit<32> address;
    ...
}

struct headers {
    ipv4 ip;
}

parser p(packet_in b, out headers h, inout standard_metadata sm) { ... }

Right now, because structs are ignored, the PI will contain a header instance field named ip.address. standard_metadata, being a struct, would ordinarily be ignored, but since we handle it specially, we'll end up with a header instance field named standard_metadata.port as well.

If we start exposing structs, the PI will automatically contain sm.port with no special handling, but if we handle structs uniformly then ip.address will now be called h.ip.address. In other words, if we want to avoid special cases, struct names would end up being a part of the fully qualified header instance field name. I've also assumed here that we'd want to use sm.port rather than standard_metadata.port - in other words, that we'd use "struct instances" rather than structs per se.

I'm not sure what gotchas or compatibility issues might arise from making this change. What do you folks think?

[mihaibudiu]

The P4-16 spec has a section on naming:
http://p4.org/wp-content/uploads/2016/12/P4_16-prerelease-Dec_16.html#sec-cp-names
The p4c-bm2-ss compiler is not fully compliant itself. Maybe we should file an issue about this.

[sethfowler]

Thanks for the link! Though it doesn't directly address structs, I think that aligns with the approach outlined in my previous comment, so it seems that from a spec point of view we'd be in a good position to start making structs visible.

And yeah, we may as well make the rest of the code conform to the spec while we're at it. =)

[mihaibudiu]

The spec does not say anything about structs because there is no assumption about the language used to express the API. This is also one reason that enums are tricky. But indeed, some user-defined P4 datatypes may have to exposed to the API.

[sethfowler]

I wasn't proposing to make structs visible by expressing them as "structs" at the API level; rather, I just want to:

• Expose struct instance fields in the same way header instance fields are exposed now.
• Include structs in the fully qualified names of header instance fields in a consistent way.

This comment from earlier in the discussion gives some examples: #225 (comment)

[mihaibudiu]

I don't know anything about how the API generation works now.

But in principle you only need to expose a type to the control-plane if it is needed. This implies that table key types and action argument types are required. If none of them is a struct then this problem does not arise. We could have the architecture forbid the use of these types for key fields or for action parameters.

There is also the issue of some extern functions that communicate with the control-plane. If these are generic the control-plane may need to know about the types used to instantiate the type arguments.

[sethfowler]

We need structs for metadata. I'd guess even unions and enums can be passed to actions, right?

And yeah, the issue of the control plane API for externs seems to keep coming up in every discussion. =)

[mihaibudiu]

@sethfowler : can we close this issue?

[sethfowler]

Most of these are no longer a problem from the perspective of control plane API generation, at least, because we don't expose header fields anymore. One thing I still think should probably be desugared in a separate pass is the implementation of isValid(). That's actually the only target-specific thing left in the control plane API generation code, or at least the only one I can remember offhand.

[mihaibudiu]

We have a separate issue for isValid(), i.e., #366.
So I will close this one.