Do not allow function calls in select labels

[mihaibudiu]

I think that this is the right approach; otherwise it's very complicated to explain when the possible side-effects of the function call may take place. But the working group has to validate this restriction.

Fixes #122

[jafingerhut]

I realize the terminology used in the compiler sources might be different than in the spec, but isn't this a function, not an action, according to spec language?

[fruffy]

Yes that is a mistake that happened during the test-case reduction. Feel free to rename it.

[konne88]

Instead of disallowing all functions, could we just disallow functions with a side-effect? I found myself many times wanting to call a side-effect free function in a key, e.g. to extract a certain range of bits.

[jafingerhut]

@konne88 My proposed change to the P4_16 language spec allows implementations to use pure functions. You can see that proposed change here, which sounds to me like it is perhaps the same as what you are suggesting: p4lang/p4-spec#831

I do not know how difficult it is to recognize pure functions written in P4_16, but I would guess that at least a subset of pure functions should be straightforward to recognize at compile time.

[jnfoster]

I'm catching up on this thread late: what's bad about saying that each expression in the key property of a table t is evaluated (from left to right, as with function invocations) whenever t.apply() is invoked? Note that t.apply() is a statement, so there is a trivial compilation strategy...

[jnfoster]

In fact, assuming m.buildKey does the obvious thing -- i.e., evaluates each expression in the key property to a value, including executing any side effects -- this seems to be exactly what we say in the spec.

See the pseudocode for match-action unit execution given in Section 13.2.3.

[jafingerhut]

One could allow such things, and define their behavior precisely, as you suggest.

An additional difficulty of implementation for having such side effects in table key expressions is that it could require introducing new temporaries in the compiled code, for some targets.

I would expect it is common in some high speed targets to need to create the entire search key, i.e. the value of all search key fields, at one time, and then ship that off to some table implementation. For example, this source code:

bit<8> f1(in bit<8> a, inout bit<8> b, inout bit<8> c) {
    b = a+5;   c=a-7;
    return a >> 1;
}
bit<8> f2(in bit<8> a, in bit<8> b, inout bit<8> c, inout bit<8> d) {
    c = a+b;   d=a-b;
    return a+b;
}
bit<8> a, b, c, d;
table t1 {
    key = {
        b & 0x7 : exact @name("masked_b");
        f1(a, b, c) : exact;
        b : exact;
        f2(a, b, c, d) : exact;
    }
    // ... rest of table properties defined here, not relevant to example
}
apply {
    // assign values a, b, c, d here
    t1.apply();
}

For the third key, b, we need to calculate its updated value as modified by the call to f1 before the full table key is known, but we need to use the original value of b for the first key expression b & 0x7.

Also, we need to define whether the modifications made for out and inout parameters in table key expressions happen before invocation of actions. Perhaps that is already what comes out of the current specification most naturally, but adding an example to the spec pointing this out explicitly, for P4 compiler implementers to ensure they get right, seems warranted.

Then there is the question of abstract/virtual functions, and whether it interacts with this behavior in some way, if and when those behaviors are agreed upon.

[jnfoster]

(Sorry for inadvertently closing the issue...)

I think the spec does imply this behavior. In particular, the line m.buildKey(m.key) suggests the execution of a helper function that, presumably, takes the expressions in the key of the table and evaluates them including any side effects.

Of course using arbitrary expressions as keys will sometimes have a cost. But we are not increasing the expressiveness of the language by supporting this feature.

If f returns a value of type T then

table t {
  key = { f() : exact; }
  ...
}
...
t.apply();

is equivalent to

T x;
table t { 
  key = { x : exact }
  ...
}
x = f();
t.apply();

where x is a fresh variable. And this second form is definitely legal.

Why force the programmer to jump through hoops when the compiler can easily do this transformation for them? There are plenty of other places in P4 where we provide an abstraction that is convenient for programmers even though it may imply some extra costs.

Here, the compiler will be able to deduce that extra state is not needed in the common case (e.g., variables, and h.isValid().) And if extra state is needed and it exceeds the available resources, a good compiler will give programmers tools for figuring that out.

[jafingerhut]

I have no personal problem with a P4 compiler allowing and supporting this. The implied support seem non-obvious to me from a reading of the current spec, given that buildKey is mentioned exactly once in the spec, in pseudocode, with nothing else that says precisely what buildKey does inside of it.

If it is decided that it ought to be defined as you suggest, adding an example that makes it obvious that this is what is implied, as opposed to a judgment call, would be a good idea, including a mention that it would be correct for an implementation to perform all of the side effects of buildKey before the apply call.

[jnfoster]

Agree this is vague. Shall we discuss at the next LDWG? Seems like reaching community consensus is probably best...

[jafingerhut]

Certainly seems reasonable to discuss it at a LDWG meeting. There is already an entry in the LDWG Wiki page table for it, with links to corresponding p4-spec issue and proposed PR.

[ChrisDodd]

Should definitely add a test case that uses hdr.isValid() in a table key, as that should be allowed (but in a quick scan of testdata I could not see any testcases)

[mihaibudiu]

It looks like we will have to rework this, since the LDWG decided to allow expressions in table keys.

[mihaibudiu]

I have narrowed down the scope of this PR; we will deal with issue #2258 separately.
I think that the checks here should be uncontroversial - we just don't allow any function calls (except isValid()) in expressions that are select labels.

[mihaibudiu]

I think this is ready for review; I think that this part is somewhat less controversial in the spec.

[fruffy]

Since the scope got narrowed, is this check still correct?

[mihaibudiu]

We should probably bring this up again in the language design group. (or figure out what the conclusion was when we discussed this last time.)

[mihaibudiu]

I will remove this code, it is not necessary, since actions cannot be called in a key computation because they return void, so they cannot be part of an expression anyway.