SideEffectOrdering: Regression?

[fruffy]

Hello.

I have the following program, which I reduced to look just like the example in the spec:

bit<8> f(inout bit<8> x, in bit<8> z) {
    return 8w4;
}
bit<8> g(inout bit<8> z) {
    z = 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));
    }
}

After SideEffectOrdering, the program looks like 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 = tmp;
            f(h.h.a, tmp_0);
        }
    }
}

which implies that h.h.a is 3 instead of being unchanged.

According to section 6.7, I would expect the transformation to look something 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;     // evaluate h.h.a; save result
        tmp_0 = g(h.h.a);  // evaluate g(h.h.a); save result; modifies h.h.a
        f(tmp, tmp_0);    // evaluate f; modifies tmp
        h.h.a = tmp;         // copy inout result back into h.h.a
    }
}

so the value of h.h.a does not change since f does not modify the input.

2288.p4.txt
2288.stf.txt

[fruffy]

The sample program is missing the pkt.extract(hdr.h); call, for some reason I cannot update the original file.

[fruffy]

Just adding another instance of this issue with side-effects in table calls.

The program

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    action simple_action_0() {
        h.eth_hdr.eth_type = 16w3;
    }
    action dummy_action_0(inout bit<16> val1, in bit<16> val2) {
    }
    table simple_table_0 {
        key = {
            h.eth_hdr.src_addr: exact @name("key1") ;
        }
        actions = {
            simple_action_0();
        }
        default_action = simple_action_0();
    }
    apply {
        dummy_action_0(h.eth_hdr.eth_type, (simple_table_0.apply().hit ? 16w1 : 16w2));
    }
}

is transformed into

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    action simple_action_0() {
        h.eth_hdr.eth_type = 16w3;
    }
    action dummy_action_0(inout bit<16> val1, in bit<16> val2) {
    }
    table simple_table_0 {
        key = {
            h.eth_hdr.src_addr: exact @name("key1") ;
        }
        actions = {
            simple_action_0();
        }
        default_action = simple_action_0();
    }
    bool tmp;
    bit<16> tmp_0;
    bit<16> tmp_1;
    apply {
        {
            tmp = simple_table_0.apply().hit;
            if (tmp) {
                tmp_0 = 16w1;
            } else {
                tmp_0 = 16w2;
            }
            tmp_1 = tmp_0;
            dummy_action_0(h.eth_hdr.eth_type, tmp_1);
        }
    }
}

which also misses the copy-out.

2288b.p4.txt
2288b.stf.txt

[jafingerhut]

@fruffy I do not see what is wrong with the after vs. before code you describe in this comment: #2288 (comment)

It is synthesizing code to evaluate the expression (simple_table_0.apply().hit ? 16w1 : 16w2) before the call to dummy_action_0, which makes sense, and then it makes the call to dummy_action_0, and its first parameter is direction inout, so any copy-in-copy-out behavior implied by the inout direction of dummy_action_0's first parameter is still present in the second code snippet, isn't it?

Or perhaps what I am missing here is "what should the initial value of the first parameter of dummy_action_0 when it starts executing? That is, should it be the initial value of h.eth_hdr.eth_type before simple_table_0.apply() is executed, or should it be the value after simple_table_0.apply() is executed. I really, really hope no one writing understandable P4 code ever writes something like that -- it is too confusing.

[fruffy]

Well simple_table may call simple_action_0 which modifies h.eth_hdr.eth_type to be 16w3. This goes against the order of evaluation as defined in the spec.
According to the example in the spec this should look like:

tmp = h.eth_hdr.eth_type;
tmp_0 = simple_table_0.apply().hit;
if (tmp_0) {
    tmp_1 = 16w1;
} else {
    tmp_1 = 16w2;
}
tmp_2 = tmp_1;
dummy_action_0(tmp, tmp_2);
h.eth_hdr.eth_type = tmp;         // copy inout result back into h.eth_hdr.eth_type

So that the initial value of h.eth_hdr.eth_type is preserved.

Edit: Just saw the update. Yeah the code is very odd, the extern example is much more convincing. But it is the same kind of problem as discussed in Section 6.7. table.apply().hit in general is fairly nasty because it can have all sorts of side effects. Compared to functions, actions can also modify control global variables.

[jafingerhut]

So in your proposed modified code in this comment: #2288 (comment) note that dummy_action_0's copy-in and copy-out behavior is applied to the new variable tmp, whereas in the original it applies to h.eth_hdr.eth_type. What I don't understand clearly from what the spec says is: what should the final value of h.eth_hdr.eth_type be? Two semi-reasonable-sounding possibilities occur to me:

(a) it should be the same as it was before the call to dummy_action_0(h.eth_hdr.eth_type, (simple_table_0.apply().hit ? 16w1 : 16w2)); in the original code before transformation, because of the copy-in copy-out behavior applied to h.eth_hdr.eth_type.

(b) it should be as modified by the call to simple_table_0.apply()'s action, if its action modifies that field.

[fruffy]

My bad. The code was missing the final copy-out line. With that the result should be whatever modification has been applied to tmp. Any side-effect that happens in simple_table_0.apply().hit should be ignored.

[jafingerhut]

And if we can get confused about it, that is the reason why I have sometimes mentioned that I would consider it semi-reasonable if a P4 compiler (at least the front end) rejected such programs as errors, because it depends so subtly upon the order of evaluation. If not that (which I know is probably a kind of odd thing to propose that a language actually treat such things as errors, versus warnings or unspecified behavior), then at least have a lint tool that can warn programmers about such strange things.

[fruffy]

I personally think it would make sense to prohibit method calls with side-effects as function call arguments. They can cause a lot of confusion and pain (in addition, there are also things like exit calls). I can not think of a case where allowing them is beneficial? But I have no big stake in this.

[mihaibudiu]

I appreciate @milicaperic . I am reviewing this.