Compiler Bug in simplifySwitch.cpp

[kfcripps]

Building the following P4 program:

extern void __e(in bit<28> arg);
extern void __e2(in bit<28> arg);

control C() {
    action bar(bool a, bool b) {
        bit<28> x; bit<28> y;
        switch (a) {
        b: {
                __e(x);
            }
        default: {
            if (b) {
                __e2(y);
            }
            }
        }
    }

    action baz() {
        bar(true, false);
    }

    action foo() {
        baz();
        baz();
    }

    table t {
        actions = { foo; }
        default_action = foo;
    }

    apply {
        t.apply();
    }
}

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

top(C()) main;

results in:

In file: p4c/frontends/p4/simplifySwitch.cpp:34
Compiler Bug: t.p4(7): Unexpected expression a;
        switch (a) {
                ^

[Vineet1101]

@kfcripps the issue you are facing is most probably because many languages take arg of switch as int or enum not bool. So you can use if-else instead of switch statement here

[kfcripps]

@Vineet1101 I don't think that's the problem here.. Attached below is a simplified reproducer, which has a switch expression of type bit<16> instead of bool, and results in the same crash:

extern void __e(in bit<28> arg);

control C() {
    action bar(bit<16> a) {
        switch (a) {
            2:       { __e(2); }
            default: { __e(3); }
        }
    }

    action foo() {
        bar(3);
    }

    table t {
        actions = { foo; }
        default_action = foo;
    }

    apply {
        t.apply();
    }
}

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

top(C()) main;

[kfcripps]

Also, if switch expressions cannot be bool, then that should result in an error message instead of a Compiler Bug.

[fruffy]

@kfcripps the issue you are facing is most probably because many languages take arg of switch as int or enum not bool. So you can use if-else instead of switch statement here

Anything that throws a "Compiler Bug" is a problem in the compiler that needs to be fixed.

[Vineet1101]

@kfcripps the P4_16 specification (at least for versions up to v1.2.1) requires that the switch expression be of the form derived from a table apply—so that the case labels correspond to action names. When you try to switch on “a” (a plain boolean) and label the case with the identifier “b” (which in this context is not a constant expression but a variable name), p4c doesn’t know how to process it and fails with an “Unexpected expression a” similarly in the next example which you gave switch is applied using an arbitrary expression( bit<16> a) . This unsupported use causes the internal compiler logic to encounter an unexpected expression and crash. here's the document
https://github.com/p4lang/p4-spec/blob/main/p4-16/spec/docs/implementing-generalized-switch-statements.md

[kfcripps]

the P4_16 specification (at least for versions up to v1.2.1) requires that the switch expression be of the form derived from a table apply

That's no longer the case. The spec is currently a bit unclear with respect to this feature, but generalized switch statements in actions are allowed as of p4lang/p4-spec#945 (please see https://p4.org/wp-content/uploads/2024/10/P4-16-spec-v1.2.5.html#sec-switch-statement-with-integer-or-enumerated-type-expression as well as p4lang/p4-spec#1363 and related discussion).

In fact, there exists test issue3299.p4, which shows that the p4c frontend passes produce valid IR for a switch statement with a bit<8> expression located in an action body. So the second program that I shared should successfully compile as well (the first one should not compile, but for a different reason - the b label is not a compile-time known value, and the expression cannot be of type bool).

[kfcripps]

The problem is the following:

The type checker marks a as a compile-time constant on these lines, because it is an action param with no direction:

        } else if (paramDecl->direction == IR::Direction::None) {
            setCompileTimeConstant(expression);
            setCompileTimeConstant(getOriginal<IR::Expression>());
        }

DoSimplifySwitch tries removing switch expressions when the switch expression is a compile-time constant. DoSimplifySwitch::postorder(IR::SwitchStatement *stat) sees that the switch expression a is marked as a compile-time constant, and calls DoSimplifySwitch::matches which fails because a is not actually a compile-time constant.

I suspect that other things might break if the type checker does not mark directionless params as compile-time constants, so at the moment I'm not sure how to fix it (maybe DoSimplifySwitch::postorder(IR::SwitchStatement *stat) just needs to not use typeMap->isCompileTimeConstant() to determine whether the switch expression is a compile-time constant).

[jafingerhut]

@kfcripps the P4_16 specification (at least for versions up to v1.2.1) requires that the switch expression be of the form derived from a table apply—so that the case labels correspond to action names. When you try to switch on “a” (a plain boolean) and label the case with the identifier “b” (which in this context is not a constant expression but a variable name), p4c doesn’t know how to process it and fails with an “Unexpected expression a” similarly in the next example which you gave switch is applied using an arbitrary expression( bit<16> a) . This unsupported use causes the internal compiler logic to encounter an unexpected expression and crash. here's the document https://github.com/p4lang/p4-spec/blob/main/p4-16/spec/docs/implementing-generalized-switch-statements.md

I am not sure if there is confusion on this point, but I wanted to point it out, in case there is.

Note the following sentence near the beginning of the article you linked to:

"This article is not part of the P4_16 language specification because it is simply one possible way to implement these generalized switch statements."

There are many ways to implement switch statements in p4c.

I wrote that article since it seemed to be a relatively low effort strategy for generalizing switch statements in p4c and behavioral-model implementations at the time, because they both already had support for the switch (tablename.apply().action_run) kind of switch statement, but not yet the generalized ones.

If someone wants to support switch (expr) statements in P4 actions, where expr is not of the form tablename.apply().action_run, then the current technique used by p4c will not work there, because tablename.apply() calls are not supported in actions, and there is no desire by anyone I am aware of to generalize actions to allow that.