Even in arguments must be saved to temporaries if aliased

[mihaibudiu]

This fix is inspired by #2638, but it is simpler; it tweaks the existing function mayAlias instead of creating a new case to handle functions.

Fixes #2288.

Amazingly, there was a test called side_effects.p4 which was designed to test exactly these cases and the output produced for that test result was wrong! Hopefully people do not write such confusing programs in practice.

[milicaperic]

I have tested your code for example

bit<8> f(in bit<8> x, in bit<8> y, out bit<8> z) {
    z = x | y;
    return 8w4;
}

bit<8> g(out bit<8> w) {
    w = 8w3;
    return 8w1;
}
control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {

    apply {
        f(h.h.a, g(h.h.a), h.h.b);
    }
}

after SideEffectOrdering your code transformed to

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bit<8> tmp;
    bit<8> tmp_0;
    apply {
        {
            tmp = g(h.h.a);
            tmp_0 = tmp;
            f(h.h.a, tmp_0, h.h.b);
        }
    }
}

but we expect the transformation look like

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bit<8> tmp;
    bit<8> tmp_0;
    apply {
        {
            tmp = h.h.a;
            tmp_0 = g(h.h.a);
            f(tmp, tmp_0, h.h.b);
        }
    }
}

Are you going to post another issue with this?

[mihaibudiu]

You are right, this requires a recursive traversal of the argument expressions. I will try to post a fix that takes this insight into account.

[mihaibudiu]

Here is another attempt to solve this problem. The solution is to compute all modified expressions in a call. These must be just left-values. Then scan all arguments and use temporaries for all which alias with one of the modified expressions.

I have also realized that calls that invoke tables were not handled properly; such calls will invoke actions, and the actions can mutate all variables in scope. In the absence of a precise interprocedural analysis the conservative solution is to just copy in and out all arguments when one of them is a table invocation. Here is an example:

table t { ... }
f(t.apply().hit ? x : y, z);

Since we don't know in this analysis what t.apply() will mutate, z must be copied in and out.

[milicaperic]

What is the conclusion of discussion [https://github.com/p4lang/p4-spec/issues/867] about order-dependent constructs?
Is the expected transformation after SideEffectOrdering in this example

bit<8> g(inout bit<8> z) {
    z = 8w3;
    return 8w1;
}

bit<8> f(in bit<8> z, inout bit<8> x) {
    return 8w4;
}
control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {

    apply {
       
               f(g(h.h.a), h.h.a);
    }
}

this:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bit<8> tmp;
    bit<8> tmp_0;
    apply {
        {
            tmp = g(h.h.a);
            tmp_0 = h.h.a;
            f(tmp, tmp_0);
            h.h.a = tmp_0;
        }
    }
}

or this?

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bit<8> tmp;
    bit<8> tmp_0;
    apply {
        {
            tmp = h.h.a;
            tmp_0 = g(h.h.a);
            f(tmp_0, tmp);
            h.h.a = tmp;
        }
    }
}

[milicaperic]

I have one more question.
What will happen with this:
f(s[a].z, g(a)) ?

I have looked at specification and there is:

bit tmp1 = a;			// save the value of a
bit tmp2 = s[tmp1].z;       // evaluate first argument
bit tmp3 = g(a);		// evaluate second argument; modifies a
f(tmp2, tmp3);			// evaluate f; modifies tmp2
s[tmp1].z = tmp2;		// copy inout result back; dest is not s[a].z

Your code transformed to this

tmp = s_0[a_0].z;
tmp_0 = g(a_0);
f(tmp, tmp_0);
s_0[a_0].z = tmp;

What is the right transformation?

[mihaibudiu]

My result is still incorrect.

[mihaibudiu]

What is the conclusion of discussion [https://github.com/p4lang/p4-spec/issues/867] about order-dependent constructs?

I don't think that the discussion has reached a conclusion.

Is the expected transformation after SideEffectOrdering in this example

I think that the spec says you have to first evaluate the first argument, which means that you need to call g(h.h.a), which includes mutating h.h.a, before you evaluate the second argument. So that means that the first translation is the correct one.

[fruffy]

The current failure in Gauntlet is a false positive related to header indices. Those are tricky to get right because copy-in/copy-out has to be performed on symbolic indices.

This is also the reason why the bug in side_effects.p4 was not caught (as far as I can tell). It should be fixed now. I will fuzz indices in the next couple of days. I can also fuzz this pull request extensively.

[fruffy]

I generated and tested 10000 programs for this particular pull request and did not find any new issues.

[fruffy]

This pull request has been sitting here for a while. I can't comment on the concrete code changes because I am lacking the context but I am approving this based on the fact that extensive fuzzing has not revealed any issues with these changes.