Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kfcripps opened this issue Oct 13, 2024 · 4 comments
Assignees
Labels
bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser)

Comments

@kfcripps
Copy link
Contributor

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: #4948

@kfcripps kfcripps added bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser) labels Oct 13, 2024
@ChrisDodd
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

kfcripps commented Oct 13, 2024

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
Copy link
Contributor Author

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):

  • A write to an l-value should clobber all previous writes to slices of the same l-value
  • A read of some l-value should be considered as a read of all previously written slices of the same l-value
  • A write to an l-value slice should be treated as a write to that specific slice only
  • A read of a sliced l-value should be considered as a read of the whole l-value (and therefore as a read of all previously written slices of the same l-value)
  • The SliceTracker logic should be moved to its own separate pass (this is where we can more carefully track each read/written bit individually)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

No branches or pull requests

4 participants