p4lang / p4-spec

Apache License 2.0
178 stars 80 forks source link

Allow compile-time resolvable expressions for switch labels? #1281

Open fruffy opened 6 months ago

fruffy commented 6 months ago

Prompted by the discussion in https://github.com/p4lang/p4c/pull/4657.

Currently, this program is illegal according to the P4 specification:

action a(){}
action b(){}
action NoAction(){}
control c()
{
  table t  {
    actions = { a ; b;  }
  }
  apply {
    switch(t.apply().action_run) {
      true ? a : b : {}
    }
  }
}

There was a fix in the compiler that made it illegal: https://github.com/p4lang/p4c/issues/3622 The spec reason is: All switch labels must be names of actions of the table t, or default.

Should this actually be illegal? I currently can not think of a case where this would be troublesome. It might even be useful to change labels because of a compile-time value change?

vlstill commented 6 months ago

For actions the problem is (I guess) that there is no type for the expression result. Allowing this would require introducing something like actionLabel<table_t> type or at least actionLabel (or action). If you want to allow const_bool ? a : b then it would be only natural to allow e.g. fun(constants...) as label. But then you have to say the type of fun, which has to be essentially action label (as the switch does not care about signature of the action, i.e. number of arguments). The biggest quiestion would be if the action label type should be parametrized by the table it belong to. To me, it would make more sense for it to have it in the type. That way we would be able to type-check the whole expression properly:

control c()
{
  action a(){}
  action b(){}
  action c(){}
  table t1  {
    actions = { a ; b; }
  }
  table t2 {
    actions = { a; c; }
  }

  apply {
    switch(t.apply().action_run) {
      true ? a : c : {}
    }
  }
}

In this case, if we check only after constant folding, we would accept the program. But I would propose the program is ill-formed as if I replace true with another value of the same type I get an ill-formed program. I think we should properly type-check such expressions.

Of course, the complication with something like actionLabel<table_t> is that now we have essentially an "object" (in a loose sense not in a sense of object-oriented programming) as a generic argument. In the P4C, there is already Type_Table so maybe this would not be a big stretch, but I don't see a "type of table" (i.e. a type-level entity for the table declaration) in the standard (at a glance).

Furthermore, currently we would be in practice mostly using this with expressions, as functions cannot be declared inside control and actions often are. So we would not be able to return a action label from a function declared before the control that contains the `action.


So overall I would say this is an interesting idea, and I could see it useful in some cases to pull some parameters out of the control block, but I think it would not be a small change in the spec to do it properly. Therefore it would be nice to have a real use case.

jafingerhut commented 6 months ago

While the particular example given in the issue has actions as the switch case labels, there are also switch statements that can other types as the case labels, e.g. bit<W> and some other types. Is that in scope for this question?

If so, is it weird to consider that expressions would be allowed for types that already have expressions defined for them in the language, and not introduce new expressions whose result is an action name?

Or is the question "should we add expressions whose result is an action name?"

fruffy commented 6 months ago

While the particular example given in the issue has actions as the switch case labels, there are also switch statements that can other types as the case labels, e.g. bit and some other types. Is that in scope for this question?

They should also be foldable if they are compile-time known. I believe #4656 already explores this.

The spec is relatively flexible for the expression type of a switch label: https://p4.org/p4-spec/docs/P4-16-working-spec.html#sec-switch-stmt

The only restriction is that for action_run the match cases must be existing actions. To represent this the compiler currently uses a PathExpression with Type_ActionEnum. But it could also just be a StringLiteral with Type_ActionEnum.

vlstill commented 6 months ago

Or is the question "should we add expressions whose result is an action name?"

I would like it if the question was interpreted that way :-).

That is a way of allowing this that would be consistent with the rest of the spec. But of course, such expressions must have type (that should be expressible in P4 I'd say).

The only restriction is that for action_run the match cases must be existing actions. To represent this the compiler currently uses a PathExpression with Type_ActionEnum. But it could also just be a StringLiteral with Type_ActionEnum.

Frankly, I would not consider how this is implemented too much when speaking about the spec. It definitely must be implementable, but the exact implementation is up to discussion on the compiler side. The spec does not map to the compiler directly after all (e.g. there are not path expressions in the spec).

fruffy commented 6 months ago

Frankly, I would not consider how this is implemented too much when speaking about the spec. It definitely must be implementable, but the exact implementation is up to discussion on the compiler side. The spec does not map to the compiler directly after all (e.g. there are not path expressions in the spec).

What I want to point out is that the spec already contains support for such expressions in the form of nonBraceExpression. Internally, the compiler implements action names using PathExpressions.

Unless you want to explicitly add a new element that is an ActionLabel which can only be used in the context of switch statements with an action_run expressions or tables. I am not sure we need this.