p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
671 stars 443 forks source link

`SimplifyDefUse` should not consider assignments to slices to be "reads" of the sliced l-value #4962

Open kfcripps opened 1 week ago

kfcripps commented 1 week ago

Building the following P4 program:

#include <core.p4>

parser p(packet_in packet, out bit<8> f) {
    state start {
        f[3:0] = 2;
        f[7:4] = 3;
        transition accept;
    }
}

parser simple(packet_in packet, out bit<8> f);
package top(simple e);
top(p()) main;

results in the following warning:

tmp4.p4(5): [--Wwarn=uninitialized_use] warning: f may be uninitialized
        f[3:0] = 2;
        ^

It doesn't make sense to print this kind of warning about a write to an uninitialized field. The code in SimplifyDefUse considers these assignments to "read" the sliced l-value f, but this is a gross hack and the logic should be rewritten in a more sensible way. Doing so should result in this unhelpful warning going away.

A similar problem was recently fixed in the ParsersUnroll pass: https://github.com/p4lang/p4c/pull/4948

ChrisDodd commented 1 week ago

It's not actually that it considers sliced assignments to be "reads" -- its that it considered sliced assignments to not replace all previous writes to the variable. Fixing this would require tracking each bit of each variable individually, instead of just tracking the variable as a whole.

kfcripps commented 1 week ago

its that it considered sliced assignments to not replace all previous writes to the variable.

My understanding is that the way it does this is by marking the slice assignment as a "read" of the sliced l-value, so that previous assignments to the same l-value will not be removed: https://github.com/p4lang/p4c/blob/main/frontends/p4/simplifyDefUse.cpp#L1396-L1402

As a side-effect, we get this superfluous warning.

kfcripps commented 1 week ago

Fixing this would require tracking each bit of each variable individually, instead of just tracking the variable as a whole.

I agree, that's basically what needs to be done for this issue. I spent some time trying something like this but I gave up as I couldn't get it to work initially and didn't want to spend anymore time working on it.

kfcripps commented 1 week ago

Fixing this would require tracking each bit of each variable individually, instead of just tracking the variable as a whole.

Alternatively, we can just be conservative in SimplifyDefUse (but the current behavior of treating writes as reads is too conservative):