Local Copy Propogation issue

[komaljai]

void ipip_push(inout headers_t hdr, in metadata_t meta)
{
   hdr.inner = hdr.outer;
   hdr.outer.srcAddr = meta.src;
   hdr.outer.dstAddr = meta.dst;
   hdr.outer.ttl = 64;
   hdr.outer.protocol = 4; /* IPIP */
   /* Assume MTU can accomodate +20 bytes */
   hdr.outer.totalLen = hdr.outer.totalLen + 20;
   hdr.outer.hdrChecksum = 0;
}

control Deparser(
    packet_out pkt,
    inout    headers_t hdr,
    in    metadata_t meta,
    in    pna_main_output_metadata_t ostd)
{
    apply {
        pkt.emit(hdr.ethernet);
	if (meta.push && hdr.outer.isValid()) {
               /* Push the ipip header */
               ipip_push(hdr, meta);
	}
    }
}

The IR representation at end of frontend looks like -

control Deparser(packet_out pkt, inout headers_t hdr, in metadata_t meta, in pna_main_output_metadata_t ostd) {
    @name("Deparser.hdr_0") headers_t hdr_1;
    @name("Deparser.meta_0") metadata_t meta_1;
    apply {
        pkt.emit<ethernet_t>(hdr.ethernet);
        if (meta.push && hdr.outer.isValid()) {
            hdr_1 = hdr;
            meta_1 = meta;
            hdr_1.inner = hdr_1.outer;
            hdr_1.outer.srcAddr = meta_1.src;
            hdr_1.outer.dstAddr = meta_1.dst;
            hdr_1.outer.ttl = 8w64;
            hdr_1.outer.protocol = 8w4;
            hdr_1.outer.totalLen = hdr_1.outer.totalLen + 16w20;
            hdr_1.outer.hdrChecksum = 16w0;
            hdr = hdr_1;
        }
    }
}

The issue is copy propogation, 'hdr' is assigned to 'hdr1' in beginning and 'hdr1' is assigned to 'hdr' back at the end, Shouldn't this be optimized?

[ChrisDodd]

It is not incorrect, just inefficient.

The copy is introduced by inlining which is not smart enough to determine that hdr (argument to Deparser) cannot possibly be modified/accessed by code in the ipip_push function. Improving the code I added in #4877 might be able to fix this.

LocalCopyProp is likewise not smart enough to do this. It might propagate the hdr_1 = hdr assignment into the first inner assignment (making it hdr_1.inner = hdr.outer), but that would kill the copy (hdr1 partly overwritten) so would not be able to copyprop any of the subsequent uses.

[grg]

From an end user's perspective, there's currently an inconsistency between how controls and functions are inlined.

Compiling the code referenced by @komaljai yields this at the end of FrontEnd:

control Deparser(packet_out pkt, inout headers_t hdr, in metadata_t meta, in pna_main_output_metadata_t ostd) {
    @name("Deparser.hdr_0") headers_t hdr_1;
    @name("Deparser.meta_0") metadata_t meta_1;
    apply {
        pkt.emit<ethernet_t>(hdr.ethernet);
        if (meta.push && hdr.outer.isValid()) {
            hdr_1 = hdr;
            meta_1 = meta;
            hdr_1.inner = hdr_1.outer;
            ...
            hdr_1.outer.hdrChecksum = 16w0;
            hdr = hdr_1;
        }
        pkt.emit<ipv4_t>(hdr.outer);
        pkt.emit<ipv4_t>(hdr.inner);
    }
}

But if we change the ipip_push function to a control, then we get:

control Deparser(packet_out pkt, inout headers_t hdr, in metadata_t meta, in pna_main_output_metadata_t ostd) {
    apply {
        pkt.emit<ethernet_t>(hdr.ethernet);
        if (meta.push && hdr.outer.isValid()) {
            hdr.inner = hdr.outer;
            ...
            hdr.outer.hdrChecksum = 16w0;
        }
        pkt.emit<ipv4_t>(hdr.outer);
        pkt.emit<ipv4_t>(hdr.inner);
    }
}

The core of the inlining is performed by Inline for the control case and InlineFunctions for the function case.

[ChrisDodd]

From an end user's perspective, there's currently an inconsistency between how controls and functions are inlined.

This is because controls cannot be nested in controls (or functions) and functions cannot be nested in functions, but functions CAN be nested in controls. So the code to not introduce the copies would have to have an extra check that the function was not nested in the control.

[jafingerhut]

This is because controls cannot be nested in controls (or functions) and functions cannot be nested in functions, but functions CAN be nested in controls. So the code to not introduce the copies would have to have an extra check that the function was not nested in the control.

Do you have a short example of P4 code that p4c compiles today, where a function is nested in a control? I tried to create one, but get an error if I attempt to define a function inside the body of a control.

Maybe a so-called "abstract function" that has support, originally for associating a behavior with TNA register actions? Example: https://github.com/p4lang/p4app-switchML/blob/main/dev_root/p4/workers_counter.p4#L27-L39