frontends: fix overwriting definitions bug in defuse analysis

[mvido]

Fixes #2900.

Frontend pass P4::KeySideEffect called by P4::SideEffectOrdering moves
key fields computations prior to the table application when
key computation involves side effects which happens f.e. when there
is a function call (as in the sample program which demonstrates
the bug).

In our sample program MainControlImpl:

control MainControlImpl(
    inout headers_t  hdr,
    inout main_metadata_t meta,
    in    pna_main_input_metadata_t  istd,
    inout pna_main_output_metadata_t ostd)
{
    table clb_pinned_flows {
        key = {
            // other key fields also possible, e.g. VRF
            SelectByDirection(istd.direction, hdr.ipv4.srcAddr,
                                              hdr.ipv4.dstAddr):
                exact @name("ipv4_addr_0");
            SelectByDirection(istd.direction, hdr.ipv4.dstAddr,
                                              hdr.ipv4.srcAddr):
                exact @name("ipv4_addr_1");
            hdr.ipv4.protocol : exact;
        }
        actions = {
            NoAction;
        }
        const default_action = NoAction;
    }

    apply {
        if (TxPkt(istd) && pass_1st(istd)) {
            clb_pinned_flows.apply();
        } else if (TxPkt(istd) && pass_2nd(istd)) {
            clb_pinned_flows.apply();
        }
    }
}

is transformed to:

control MainControlImpl(inout headers_t hdr, inout main_metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) {
    bit<32> key_0;
    bit<32> key_1;
    bit<8> key_2;
    @name("clb_pinned_flows") table clb_pinned_flows_0 {
        key = {
            key_0: exact @name("ipv4_addr_0") ;
            key_1: exact @name("ipv4_addr_1") ;
            key_2: exact @name("hdr.ipv4.protocol") ;
        }
        actions = {
            NoAction();
        }
        const default_action = NoAction();
    }
    bool tmp;
    bool tmp_0;
    bool tmp_1;
    bool tmp_2;
    bool tmp_3;
    bool tmp_4;
    apply {
        {
            tmp = TxPkt(istd);
            if (!tmp) {
                tmp_0 = false;
            } else {
                tmp_1 = pass_1st(istd);
                tmp_0 = tmp_1;
            }
            if (tmp_0) {
                key_0 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr);
                key_1 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.dstAddr, hdr.ipv4.srcAddr);
                key_2 = hdr.ipv4.protocol;
                clb_pinned_flows_0.apply();
            } else {
                tmp_2 = TxPkt(istd);
                if (!tmp_2) {
                    tmp_3 = false;
                } else {
                    tmp_4 = pass_2nd(istd);
                    tmp_3 = tmp_4;
                }
                if (tmp_3) {
                    key_0 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.srcAddr, hdr.ipv4.dstAddr);
                    key_1 = SelectByDirection<bit<32>>(istd.direction, hdr.ipv4.dstAddr, hdr.ipv4.srcAddr);
                    key_2 = hdr.ipv4.protocol;
                    clb_pinned_flows_0.apply();
                }
            }
        }
    }
}

We can see that the same assignments are before each table apply()
method call.

In fact, in the IR, the same assignments before each apply() method
call are represented by identical IR::AssignmentStatement(s).

Then in P4::AllDefinitions::setDefinitionsAt() when we check, if
we do not overwrite the definitions for a P4::ProgramPoint for
the second assignment statement, the P4::ProgramPoint is equal
to the P4::ProgramPoint of the first assignment statement and thus
the BUG_CHECK fails.

The proposed solution is to change P4::KeySideEffect pass to create
equivalent but not identical IR::AssignmentStatement(s) before each
table apply() method call instead of identical
IR::AssignmentStatement(s) using the clone() method.

[mihaibudiu]

This kind of bug hits periodically, because indeed the IR is a DAG and not a tree, but I personally often forget that.
If this is ready for review it should not be a draft. Do you know why some tests are failing?

[mvido]

So what do you think about this solution? Is it possible to change the IR structure this way so that it does not cause problem for defuse analysis, or do we need to fix the defuse analysis so that it works also if it is not a tree?

I made it a draft, because I wanted to add a test (which was failing because run-p4-sample.py was trying to compile output from midend, and there is reported an error when you try to recompile output from midend for that test, so I needed to update run-p4-sample.py), and I wanted to check if the CI passes, and obviously it is not. I don't know why those tests are failing, I'm investigating.

[mihaibudiu]

The analysis cannot be changed, it keeps a hashmap from IR nodes to properties. If the same IR node has two different properties in different contexts it cannot be solved. Did you notice that #2971 is solving the same problem?

[mihaibudiu]

I will close this since we have merged #2971. Thank you for the fixes.

[mvido]

I haven't noticed when writing the last comment. Main thing is that it is resolved, thanks.