Compiler Bug: write set already set

[liao123123123]

The command "p4test test.p4 -v" is used to compile the following code：

#include <core.p4>

bit<16> fun1(bit<8> args1) {
    return (bit<16>)args1;
}
bit<8> fun2(bit<4> args2) {
    fun1(8w10);
    return 8w0;
}

control ingress() {
    apply {
        fun2(4w3);
        fun2(4w3);
    }
}

control Ingress();
package top( Ingress ig);
top( ingress()) main;

result in :

Compiler Bug: test.p4(4): Expression (bit<16>)10; write set already set
    return (bit<16>)args1;
           ^^^^^^^^^^^^^^

I would like to know if there is an issue with my usage.
my p4c version:p4c 1.2.4.9

I would appreciate your assistance with GitHub. Thank you.

[fruffy]

May be a duplicate of #4500.

[kfcripps]

Unfortunately, #4539 does not fix this, so it doesn't seem to be a duplicate of #4500.

[kfcripps]

If this was also introduced by e60cb8a, then it may make sense to disable the problematic second SimplifyDefUse pass for now. This is what we did in our fork as we have been experiencing many problems with that pass (including significant increases to memory usage and compile times, in addition to the #4500 and #4507 crashes).

[fruffy]

Transform is a very expensive visitor. It can copy the entire IR AST if small leaf elements like Path Expressions or members are cloned.

[fruffy]

Another thing that could lead to the memory blowup is the use of postorder. I have noticed that for complex programs using postorder can lead to a strange memory leak. This could probably be reproduced with a simple pass that just transforms a type etc.

I haven't investigated deeper. Might be worthwhile to track this in a separate issue.

[asl]

Transform is a very expensive visitor. It can copy the entire IR AST if small leaf elements like Path Expressions or members are cloned.

It's not the Transform or postorder problem. use-def is very heavy in memory allocations. Essentially almost everything ends in some memory allocation (see e.g. Definitions::joinDefinitions or LocationSet::join as a typical example).

[kfcripps]

I have confirmed that this was also introduced by e60cb8a.

[fruffy]

Should we create an umbrella issue for all these regressions? It looks like reverting the PR is the way to go. I can also confirm that I have not enabled P4Smith tests because they all produce similar problems: https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/smith/scripts/compilation-test.sh#L51

DefUse should not be this sensitive to its location in the compiler though, something else is amiss here.

[kfcripps]

@fruffy I think it would be reasonable to revert the offending PR, and then create an umbrella issue for the purpose of resolving all of these issues and re-enabling the second SimplifyDefUse pass later.

If you agree I can try reverting e60cb8a.

[fruffy]

We can do that, but the PR should add all the issue programs as samples, too. It should also keep the program bug that was fixed (maybe add it to errs as long as the issue remains open?).

[kfcripps]

We can do that, but the PR should add all the issue programs as samples, too. It should also keep the program bug that was fixed (maybe add it to errs as long as the issue remains open?).

Yes of course, sorry, by "revert" I just mean we should remove the second SimplifyDefUse pass. You are correct that e60cb8a fixed bugs in the pass and those fixes should remain in place. Added test programs should remain as well (if any).

[kfcripps]

the PR should add all the issue programs as samples, too

I can do that as well.

[kfcripps]

It should also keep the program bug that was fixed (maybe add it to errs as long as the issue remains open?).

Which bug did it actually fix? #3508?

[fruffy]

Yes of course, sorry, by "revert" I just mean we should remove the second SimplifyDefUse pass. You are correct that e60cb8a fixed bugs in the pass and those fixes should remain in place. Added test programs should remain as well (if any).

Understood, I just want to preserve the example programs that have been added because of these issues in one way or the other.

It should also keep the program bug that was fixed (maybe add it to errs as long as the issue remains open?).

Which bug did it actually fix? #3508?

Unclear to me actually, #3508 was "fixed" but the underlying problem was not iirc. Need to check why this change was added.
@mihaibudiu Do you remember the context?

[mihaibudiu]

This is the same problem as described in #4500.
If you dump the IR tree you will notice that the 22389 subtree is reused:

          <AssignmentStatement>(27087)
            <PathExpression>(27088)
              retval_0
            <Cast>(22389)
              <Constant>(2247) 10
                <Type_Bits>(1677)
              <Type_Bits>(201) */
                retval_0 = (bit<16>)8w10;
            }

          <AssignmentStatement>(27129)
            <PathExpression>(27130)
              retval_4
            <Cast>(22389)
              <Constant>(2247) 10
                <Type_Bits>(1677)
              <Type_Bits>(201) */
                retval_4 = (bit<16>)8w10;
            }

[mihaibudiu]

The fundamental problem is that there is no pass which can reliably convert an IR DAG into an IR tree.
The existing pass SimplifyDefUse::Cloner clearly doesn't handle this case.
In this case we are dealing with a constant expression (the cast) which hasn't been folded after inlining.
So we can manage it by inserting an additional constant folding pass after inlining.
But this doesn't mean this bug won't surface again.

[kfcripps]

@mihaibudiu Note also #4385 (comment).

Possible solutions I can think of right now:

1. Insert a pass before the second SimplifyDefUse pass that converts a DAG into a tree, as you have mentioned
2. Adjust SimplifyDefUse so that it can correctly operate on DAGs (not sure if actually possible)
3. Adjust the inlining passes to not transform tree -> DAG (might not be a robust solution if other passes also produce DAGs?)

[mihaibudiu]

The IR is always a dag and the is no way to enforce the result being a tree

[kfcripps]

@mihaibudiu (1) would certainly produce a tree. Am I missing something?

[mihaibudiu]

You cannot write a pass that enforces that the IR is a tree using the existing visitor infrastructure, because the visitor will refuse to replace a node with another one that it considers "equal". The SimplifyDefUse::Clone pass is an attempt to do (1), which has failed.

[kfcripps]

You cannot write a pass that enforces that the IR is a tree using the existing visitor infrastructure, because the visitor will refuse to replace a node with another one that it considers "equal". The SimplifyDefUse::Clone pass is an attempt to do (1), which has failed.

Ah, I see. So (2) seems like the most robust solution to me at the moment, but I'm not sure if it is possible or how difficult it would be to implement

[mihaibudiu]

Some passes maintain properties for IR nodes using maps from IR nodes to properties. This is what def-use does. The problem is that the DAG representation uses a single IR node for two subdags, but the properties attached to these subdags should be different, because they depend not only on the dag, but also on its context. So you would need to put in the map not just the node, but its context as well. So (2) is not trivial.

[ChrisDodd]

The midend def_use pass uses the context as well as the node, so can deal with DAGs just fine.

[kfcripps]

@ChrisDodd Are you referring to the ComputeDefUse pass?

[kfcripps]

Some passes maintain properties for IR nodes using maps from IR nodes to properties. This is what def-use does. The problem is that the DAG representation uses a single IR node for two subdags, but the properties attached to these subdags should be different, because they depend not only on the dag, but also on its context. So you would need to put in the map not just the node, but its context as well. So (2) is not trivial.

Will this solution work in all cases? What if a parent has multiple children, with 2 or more children being the same IR::Node*?

[mihaibudiu]

I assume @ChrisDodd will answer this question

[fruffy]

I have lost track on this. It seems fixed but what about the other related regressions?

regression Code that previously compiled correctly either no longer compiles or produces invalid results.