Unexpected "Duplicates declaration" error when P4 program contains no duplicate declarations

[kfcripps]

#include <core.p4>

struct S {
    bit<16> f;
}

extern bool foo(in bool x, in bool y);

control C(inout S s) {
    action a1() {
        if (foo(s.f == 0, false)) {
            return;
        }
    }

    @name("._xyz")
    action a2() {
        a1();
        a1();
    }

    table t {
        actions = { a2; }
    }

    apply {
        t.apply();
    }
}

control C2() {
    apply {
        S s;
        C.apply(s);
    }
}

control proto();
package top(proto p);

top(C2()) main;

results in:

[--Werror=duplicate] error: C.hasReturned: Duplicates declaration C.hasReturned

[oleg-ran-amd]

P4C may create unused actions in the output P4 IR.
Such actions not only pollute the IR but they may also be not properly handled by the passes like UniqueNames leading to problems like this one.

For example, for P4

control C() {
    @name("._xyz")
    action a() {}   

    table t1 { actions = { a; } }
    table t2 { actions = { a; } }

    apply {
        t1.apply();
        t2.apply();
    }
}

control Outer() {
    apply {
        C.apply();
    }
}

At first, controls inlining renames a() to

@name("._xyz") action __xyz()

Then FindRepeatedActionUses + DuplicateActions detect two uses of __xyz() in t1 and t2 and create two copies of it, ending up with 3 actions:

@name("._xyz") action __xyz() {}    // original action, no longer used
@name("._xyz") action __xyz_0() {}  // copy 1
@name("._xyz") action __xyz_1() {}  // copy 2

__xyz() is not used now.
__ at the beginning of the action name makes RemoveUnusedDeclarations think that it is an internal identifier. Unused declarations of internal identifiers are not removed.

In the original reproducer above, the unused __xyz() declares two hasReturn variables, and UniqueNames does not make their names unique, which causes the error.

I am not sure if it is a fully correct/complete fix but the following change in unusedDeclarations.cpp removes the unused action and bypasses the error:

const IR::Node *RemoveUnusedDeclarations::process(const IR::IDeclaration *decl) {
    LOG3("Visiting " << decl);
    if (decl->getName().name == IR::ParserState::verify && getParent<IR::P4Program>())
        return decl->getNode();
-    if (decl->getName().name.startsWith("__"))
+    if (decl->externalName(decl->getName().name).startsWith("__"))
        // Internal identifiers, e.g., __v1model_version
        return decl->getNode();

Instead of checking the modified __xyz() name, it checks the original name from @name, if it exists, to see if it starts with __.

[kfcripps]

In the original reproducer above, the unused __xyz() declares two hasReturn variables, and UniqueNames does not make their names unique, which causes the error.

@oleg-ran-amd Why does it not make their names unique? Is it because the action is unused and UniqueNames only processes used actions?

[asl]

This:

At first, controls inlining renames a() to
@name("._xyz") action __xyz()

Looks potentially problematic. Do we understand the logic behind the renaming rules? Looks like it might create bunch of internal names that might confuse other places as well?

[oleg-ran-amd]

Why does it not make their names unique? Is it because the action is unused and UniqueNames only processes used actions?

@kfcripps I did not debug that deep. It seems the unused action brakes IR and IR::Declaration_Variable nodes in it somehow.
Here is what the two copies of a2 from the reproducer look like before renaming:

    @name("._xyz") action __xyz() {  // <===== unused copy
        {
            @name("C.hasReturned") bool C_hasReturned = false;  // <===== (1)
            C_tmp = foo(s_0.f == 16w0, false);
            if (C_tmp) {
                C_hasReturned = true;
            }
            if (!C_hasReturned) {
            }
        }
        {
            @name("C.hasReturned") bool C_hasReturned = false;  // <===== (2)
            C_tmp = foo(s_0.f == 16w0, false);
            if (C_tmp) {
                C_hasReturned = true;
            }
            if (!C_hasReturned) {
            }
        }
    }
    @name("._xyz") action __xyz_0() {
        {
            @name("C.hasReturned") bool C_hasReturned = false;  // <===== (3)
            C_tmp = foo(s_0.f == 16w0, false);
            if (C_tmp) {
                C_hasReturned = true;
            }
            if (!C_hasReturned) {
            }
        }
        {
            @name("C.hasReturned") bool C_hasReturned = false;  // <===== (4)
            C_tmp = foo(s_0.f == 16w0, false);
            if (C_tmp) {
                C_hasReturned = true;
            }
            if (!C_hasReturned) {
            }
        }
    }

It is important to note that (1) and (2) in the unused copy of action are the same IR::Declaration_Variable*s for some reason.
UniqueNames sees them as a single variable declaration and renames it. Since these declarations are the same node, both (1) and (2) get the same (new) name.

(3) and (4) are different IR::Declaration_Variable*s, so they get unique names.

Even if such an action has no duplicate symbols or other problems and compilation succeeds, the output IR after the midend still contains an unused action (and there may be more such actions), which does not look right.

[oleg-ran-amd]

Looks potentially problematic. Do we understand the logic behind the renaming rules?

@asl Yeah.
. allows to specify a global name and prevent it from mangling by P4C. On the other hand, . is not a valid character for an identifier, so P4C replaces it with _.

[asl]

Maybe the logic should be changed there? In order not to clash with internal identifiers?

Tagging @ChrisDodd @vlstill @fruffy

[fruffy]

Maybe the logic should be changed there? In order not to clash with internal identifiers?

Tagging @ChrisDodd @vlstill @fruffy

My first intuition here is to change the renaming behavior of the inlining passes. Usually, they just use the parent control/parser as prefix. It might be the safer option.

[kfcripps]

See #4900 (comment)