p4c icon indicating copy to clipboard operation
p4c copied to clipboard

Should P4_14 -> P4_16 auto-translation of parser value sets conform to P4_14 spec?

Open jafingerhut opened this issue 5 years ago • 4 comments

The P4_14 language specification, version 1.0.5, section 4.3 "Value Sets" says this:

"The run time API for updating parser value sets must allow value and mask pairs to be specified together."

This mention of value and mask pairs seems to imply that P4_14 parser value sets always support ternary matching.

If so, the current p4c auto-translation from P4_14 to P4_16 supported via a command like this:

p4test --std p4-14 issue946.p4 --pp issue946-auto-xlated.p4

produces a P4_16 program that uses a value_set, but in a way that only supports exact match of entries added to the value set.

I do not know if this is considered a bug worth changing or not, but P4_16 does define the @match(ternary) syntax for annotating a field of a struct that can be used to achieve what appears to be the expected P4_14 behavior. For the test program p4c/testdata/p4_14_samples/issue946.p4, it currently translates into a P4_16 program with this definition for the P4_16 value_set:

    @name(".pvs") value_set<bit<16>>(4) pvs;

whereas this would appear to be a translation that more closely matches the P4_14 program behavior:

// ... somewhere at the top level of the P4_16 program ...
struct pvs_1_type_t {
    @match(ternary)
    bit<16> f1;
}

// ... later in the parser where the value_set is instantiated
    @name(".pvs") value_set<pvs_1_type_t>(4) pvs;

jafingerhut avatar Feb 18 '20 20:02 jafingerhut

So you think this is a bug? If adding the annotation is the only thing needed the fix should be easy. What do users of p4-14 think?

mihaibudiu avatar Feb 24 '20 21:02 mihaibudiu

I phrased the issue in the form of a question, because I do not know whether most P4_14 implementations actually follow the "value set elements are ternary value/masks in the control plane API" of the P4_14 language spec, or not.

Pinging @vgurevich for his views on the question.

jafingerhut avatar Feb 24 '20 21:02 jafingerhut

In some well known implementations, they certainly are :) But the spec itself is not very specific.

vgurevich avatar Feb 25 '20 00:02 vgurevich

bmv2 does not support ternary matching for value sets at the moment, so if we make this change, all P4_14 programs using parser value sets will toggle a warning when compiled for bmv2 (https://github.com/p4lang/p4c/pull/2214) and bmv2 will discard the @match(ternary) annotation added to the P4_16 program and fallback to exact matching. I have an open issue to implement support for ternary matching but it's not a small change and I don't think I will get to it in the immediate future: https://github.com/p4lang/behavioral-model/issues/857.

That does not mean we should not do this change. I expect few people are using bmv2 with P4_14 programs...

It's strange that the default match kind (really the only possible match kind) is ternary for P4_14 but we chose exact as the default (and for a long time you couldn't override it) for P4_16.

antoninbas avatar Feb 26 '20 23:02 antoninbas