Add struct with emit to bmv2 backend

[hesingh]

The bmv2 backend implementation should be as close to the P4-16 specification as possible. The specification supports emit for a struct, but the bmv2 backend does not. We should add the support to the bmv2 backend.

See p4c/backends/bmv2/common/deparser.cpp and this code:

} else {
    ::error("%1%: emit only supports header and stack arguments, not %2%",
               arg, type);
}

[mihaibudiu]

Have you tested the program?
Structs containing headers are dismantled by another compiler pass.

[hesingh]

Here is a short test program that fails. The failure log is included first, followed by the test P4 program.

hemant@ubuntu:~/p4c-1/build$ ./p4c-bm2-ss --std p4-16 foo.p4
foo.p4(64): error: b.emit: argument must be a header, stack or union, or a struct or tuple of such types
        b.emit(h.bvh0.row);
        ^^^^^^^^^^^^^^^^^^
foo.p4(64): error: b.emit: argument must be a header, stack or union, or a struct or tuple of such types
        b.emit(h.bvh0.row);
        ^^^^^^^^^^^^^^^^^^
foo.p4(64): error: b.emit: argument must be a header, stack or union, or a struct or tuple of such types
        b.emit(h.bvh0.row);
        ^^^^^^^^^^^^^^^^^^
foo.p4(64): error: b.emit: argument must be a header, stack or union, or a struct or tuple of such types
        b.emit(h.bvh0.row);
        ^^^^^^^^^^^^^^^^^^
hemant@ubuntu:~/p4c-1/build$ 

#include <v1model.p4>

struct alt_t {
    bit<1> valid;
    bit<7> port;
};

struct row_t {
    alt_t alt0;
    alt_t alt1;
};

header bitvec_hdr {
    row_t row;
}

struct local_metadata_t {
    bit<8> row0;
};

struct parsed_packet_t {
    bitvec_hdr bvh0;
    bitvec_hdr bvh1;
};

parser parse(packet_in pk, out parsed_packet_t h, inout local_metadata_t local_metadata,
             inout standard_metadata_t standard_metadata) {
    state start {
	transition accept;
    }
}

control ingress(inout parsed_packet_t h, inout local_metadata_t local_metadata,
                inout standard_metadata_t standard_metadata) {
    apply {
        clone3(CloneType.I2E, 0, h);
    }
}

control egress(inout parsed_packet_t hdr, inout local_metadata_t local_metadata,
               inout standard_metadata_t standard_metadata) {
    apply { }
}

control deparser(packet_out b, in parsed_packet_t h) {
    apply {
        b.emit(h.bvh0.row);
    }
}

control verify_checksum(inout parsed_packet_t hdr,
inout local_metadata_t local_metadata) {
    apply { }
}

control compute_checksum(inout parsed_packet_t hdr,
                         inout local_metadata_t local_metadata) {
    apply { }
}

V1Switch(parse(), verify_checksum(), ingress(), egress(),
compute_checksum(), deparser()) main;

[mihaibudiu]

This error is provided by the front-end and is in conformance with the Section 15 of the language spec.
It has nothing to do with the bmv2 back-end.

[hesingh]

In a private workspace few weeks back, I had changed the frontend code, also the midend ExpandEmit pass, and then the bmv2 deparser gave me the error. When I get a chance during the next few days, I'd be happy to ressurect that code changes and add a log.

[hesingh]

I created this workspace to add code changes to create the bmv2 failure for emit with struct.

https://github.com/hesingh/emit-struct

In the workspace, the shorter test P4 program is included in path

testdata/p4_16_samples/emit-struct.p4

creates the problem. Please see test log below.

hemant@ubuntu:~/emit-struct/build$ ./p4c-bm2-ss --std p4-16 ../testdata/p4_16_samples/emit-struct.p4
../testdata/p4_16_samples/emit-struct.p4(16): [--Wwarn=uninitialized_out_param] warning: out parameter h may be uninitialized when parse terminates
parser parse(packet_in pk, out parsed_packet_t h, inout local_metadata_t local_metadata,
                                               ^
../testdata/p4_16_samples/emit-struct.p4(16)
parser parse(packet_in pk, out parsed_packet_t h, inout local_metadata_t local_metadata,
       ^^^^^
../testdata/p4_16_samples/emit-struct.p4(37): error: bmv2 , StructInitializerExpression: emit only supports header and stack arguments, not struct row_t
        b.emit(h.row);
               ^^^^^

[marko-stanoj]

Hello everyone,
I read your discussion and this issue seems very interesting.
There is no problem when emitting struct that does not contain fields that are base type to bmv2 backend. Problem in your example is that struct row_t, that you are trying to emit, contains fields whose type is a base type, which is not in conformance with p4-16 language specification, as said in Section 14: "When applied to a struct or header union, emit recursively invokes itself to each field. It is illegal to invoke emit on an expression of whose type is a base type, enum, or error." So I'm wondering is there still a need to add this functionality in spite of conflict with specification?

[hesingh]

See this commit to p4c which added bool and serializable enum to a struct. bool is a base type.

fd362ec

We have discussed emit and base types in this Issue. Search for "emit" in the note below to see the discussion.

p4lang/p4-spec#719

Once code is changed by fixing this Issue, then the p4-spec will also be changed for text in Section 14.

[jafingerhut]

@hesingh: In the original issue you created, you wrote:

"The bmv2 backend implementation should be as close to the P4-16 specification as possible. The specification supports emit for a struct, but the bmv2 backend does not. We should add the support to the bmv2 backend."

The P4_16 specification at this time says that you can call emit on a struct, but only if that struct contains nothing but members with type header, header stack, and/or header_union. I believe p4c and simple_switch support that already today.

If so, then the implementations are following the language spec on this issue. Do you see anything between them that differs?

[hesingh]

The bmv2 backend has an issue that it does not even allow a struct with emit as shown in a log I added. See the URL below.

#1659 (comment)

[jafingerhut]

That emit is for a struct type that contains members that are not headers, not header stacks, and not header_union. Thus the P4_16 language spec says it should be illegal. Do you agree?

[hesingh]

Ah, got it. I was only looking at the top-level emit expression which is a struct. But, then I see section 15.1 saying "emit calls itself recursively over fields in the struct". This is when emit would hit a base type which is not allowed.

Now we have a separate discussion, which is to allow emit of struct with base types, if anyone has a good use case.