Fix stale constant propagation across out/inout method arguments

[sanket-jadhav-cse]

Bug Description

GlobalCopyPropagation incorrectly preserves stale constant values for variables that are subsequently modified through method calls.

When a variable is initialized with a constant and later passed as an out/inout argument to a method, the GlobalCopyPropagation pass may fail to invalidate the variable's tracked value. As a result, the optimizer can incorrectly replace the variable with its previous constant value.

For example:

bit<32> temp = 100;
m.mutate(temp);

where mutate is declared as:

void mutate(out bit<32> val);

The optimizer may transform the call into:

m.mutate(32w100);

This is incorrect because an out argument must remain a writable lvalue.

The issue also affects member method calls such as register reads. For example:

meta.val = 10;
my_reg.read(meta.val, 0);

After the register read, meta.val no longer has the compile-time value 10. However, GlobalCopyPropagation can continue treating it as 10 and propagate the stale constant into subsequent code.

Root Cause

There are two main problems in midend/global_copyprop.cpp:

1. checkParametersForMap() uses the callee's formal parameter name instead of resolving the actual argument passed at the call site. Therefore, caller variables used as out/inout arguments are not removed from the constant-value map.

2. MethodCallExpression handling skips member methods because of the IR::Member early return. Consequently, calls such as register.read() are not considered when invalidating tracked variable values.

Impact

This can result in:

• Valid P4 programs being rejected with a type-checking error such as:
  Read-only value used for out/inout parameter.
• More seriously, stale constants being propagated after a method has modified a variable, potentially resulting in incorrect generated data-plane behavior.

This is therefore a compiler correctness bug in the GlobalCopyPropagation optimization pass.

Reproduction

A minimal example is:

extern Mutator {
    Mutator();
    void mutate(out bit<32> val);
}

control c() {
    Mutator() m;

    apply {
        bit<32> temp = 100;
        m.mutate(temp);
    }
}

Before the fix, GlobalCopyPropagation can incorrectly rewrite:

m.mutate(temp);

to:

m.mutate(32w100);

which is subsequently rejected by type checking.

Expected Behavior

GlobalCopyPropagation should invalidate the tracked value of any variable passed to an out or inout parameter and must not replace such writable arguments with constants.

Member methods that modify or produce values for variables should likewise invalidate the corresponding tracked values.

[sanket-jadhav-cse]

Does this approach look correct before I proceed with the fix and add the corresponding regression tests?

[fruffy]

From a quick glance this seems reasonable, but let's check first how this will impact the ref files.

You will see the effects of your changes in full once you apply them across the ~1000 example programs.

[sanket-jadhav-cse]

@fruffy I went ahead and validated the approach and checked the ref-file impact as suggested.

I implemented the fix for the stale out/inout propagation and added a targeted regression test covering out, inout, member-method (register.read) invalidation, and copy-in/copy-out ordering.

I also ran the full p4_16_samples suite: 1319/1319 tests passed with 0 failures.

For the ref files, no existing reference files were modified. The only added refs are for the new issue5765 regression test.

The changes are currently staged and the patch passes the relevant formatting/check validation.

Does this look good from your side, and should I open the PR?

[jafingerhut]

Out of curiosity, does your new test case give wrong output files without your fix, and correct output files with your fix?

[sanket-jadhav-cse]

Yes. I verified this against the unfixed baseline as well as the fixed build.

Without the fix, the newly added issue5765.p4 is the only failing test in the 1320-test baseline suite (1319 pass, 1 fail). The failure is the expected behavior: stale propagation rewrites the out argument to a constant, resulting in the Read-only value used for out/inout parameter error.

With the fix, the same regression test passes, and the fixed p4_16_samples suite passes 1319/1319 tests with 0 failures.

So the regression test reproduces the issue on the unfixed baseline and passes with the fix, while the existing sample tests show no regression.

[sanket-jadhav-cse]

If this approach looks correct, should I go ahead and open the PR?

[fruffy]

Sure, we will be able to judge better with the final PR.