Control instantiation parameters are not always treated as constants

[vgurevich]

Here is a simple example:

control c()(int bar=16) {
    bit<(bar)> foo;
    apply {
    }
}

Compiling this with p4test produces the following error:

param_test.p4(2): [--Werror=expected] error: bar: expected a constant
    bit<(bar)> foo;
         ^^^

[jnfoster]

I think this is consistent with the current spec. It says:

The following are compile-time known values:

• Integer literals, Boolean literals, and string literals.
• Identifiers declared in an error, enum, or match_kind declaration.
• The default identifier.
• The size field of a value with type header stack.
• The _ identifier when used as a select expression label
• Identifiers that represent declared types, actions, tables, parsers, controls, or packages.
• List expression where all components are compile-time known values.
• Structure initializer expressions, where all fields are compile-time known values.
• Instances constructed by instance declarations (Section 10.3) and constructor invocations.
• The following expressions (+, -, *, / , %, cast, !, &, |, &&, ||, << , >> , ~ , >, <, ==, !=, <=, >=, ++, [:]) when their operands are all compile-time known values.
• Identifiers declared as constants using the const keyword.
• Expressions of the form e.minSizeInBits() and e.minSizeInBytes().

I don't see constructor parameters here.

Maybe we should fix that, but then it's not a p4c bug, it's a spec bug.

[vgurevich]

@jnfoster -- I filed an issue against the spec (p4lang/p4-spec#1001) to add the constructor parameters to the list you mentioned, but note, that the spec already contains the relevant statements.

First, Section 6.7.1 says:

All constructor parameters are evaluated at compilation-time, and in consequence they must all be directionless (they cannot be in, out, or inout); this applies to package, control, parser, and extern objects. Values for these parameters must be specified at compile-time, and must evaluate to compile-time known values. See Section 14 for further details.

Section 14 says this, indeed:

Constructor parameters must be directionless (i.e., they cannot be in, out, or inout) and when the parser is instantiated, it must be possible to fully evaluate the expressions supplied for these parameters at compilation time.

If these two statements do not imply that constructor parameters are compile-time known values, I do not know what does.

[jnfoster]

I agree they are morally compile-time known.

But, to be a language spec lawyer:

• We say the expressions filled in for constructor parameters must be compile-time known values.
• But we do not say that constructor parameters are themselves compile-time known values.
  So technically p4c is doing the right thing (given the current specification).

[vgurevich]

@jnfoster -- in this case passing parameters from outer controls into the inner controls (for examples) is not allowed:

control inner()(int param = 10) {
    apply {
    }
}

control outer() (int param = 20) {
    inner(param) i;
    apply {
       i.apply();
    }
}

Should I file the bug that inner() is instantiated with the value of the parameter that is not known at compile-time? :)

[vgurevich]

@jnfoster -- also the spec says that table sizes must be compile-time only values. Should I file a bug against the compiler that currently allows this:

control c()(int param = 100) {
    action a() {}
    table t {
        size = param;
        actions = { a; }
    }
    apply {
        t.apply();
    }
}

[jnfoster]

Let me try to explain.

P4 has a set of expressions. There are infinitely many of them. They are the set of abstract syntax trees generated by the grammar we given in the appendix of the language specification. Let's call this set EXPR.

Of this infinite set of expressions, a subset are compile-time known. This set is the smallest one "closed" under the list I gave above. (So, technically, it's what's called an inductively-defined set). Let's call this set CTKV.

To typecheck a control instantiation, we insist that the arguments that are filled in for the constructor parameters be from the set CTKV.

Example:

control c()(bit<8> x) { apply { ... x ... } }
c(1 + 2) c1;

Here, the expression supplied as an argument, 1+2, is an EXPR that is also in CTKV. But the parameter x is an EXPR that is not in CTKV.

Mathematically, this is a perfectly sensible thing to do. Practically, of course, it's totally silly.

So the simple fix is to add constructor parameters to the set CTKV.

[jnfoster]

Regarding #2995 (comment), yes you should file a bug.

The compiler is not checking that the size property is compile-time known at all.

See here:

p4c/midend/checkSize.h

Line 25 in 6b635f0

class CheckTableSize : public Modifier {

The compiler front-end and mid-end also accept this program, which is wrong.

control c(inout bit<8> x)(int param = 100) {
    action a() {}
    action NoAction() {}
    table t {
        size = x;
        actions = { a; }
    }
    apply {
        t.apply();
    }
}

Probably real backends barf on it. But we should be rejecting the program earlier.

[vgurevich]

OK, so practically speaking, I think we need to fix this specific issue by assuming that p4lang/p4-spec#1001 is a done deal. Are we in agreement?

Moreover, the vast majority of the programs that use control instantiation parameters, use them to control the sizes of tables and externs. Thus, if we add the check before fixing this issue, pretty much all the code out there will be broken.

[mihaibudiu]

In principle it would be nice for this to work, but given the current structure of the compiler it's not easy. The problem is one of pass ordering: we do first constant folding, then type-checking, then type specialization for instantiations, and we never repeat. To propagate these constants you would have to put this in a loop and to modify the type-checking algorithm to allow for "constants whose values are not yet known", like these constructor parameters.

What I am saying is that the first time we typecheck the instantiation of the inner control we don't know the value of the constructor parameter.

[jnfoster]

I think we should decide if we want these to behave like macros (where any valid substitution is allowed) or like types (where the type checking is done statically and modularly). If the latter, then it may be best to keep the current design...

[vgurevich]

I would suggest making the macro-like functionality working as best as we can, since this is what is being currently used in practice (and quite widely), while leaving modularity concerns for the future, since it is not something that is being practically used/implemented and, frankly, will require tons of other things in the language that we do not have.

So, type checking will be done statically, but not modularly.

[mihaibudiu]

This is a duplicate of #2025