Added Structured Annotations

[chrispsommers]

This PR updates p4info.proto and p4types.proto as well as the P4Runtime specification to add structured annotations as described in #264 and p4lang/p4-spec#796. The P4_16 lang spec PR is not yet ratified, it is awaiting a practical implementation in p4c so one goal of this PR is to inform an implementation. Step one is to parse the syntax, step two is to generate the new P4info.

[stefanheule]

Could we use a simpler proto representation that makes it clear whether we are dealing with a list or kvpairs (like in the grammar)? Maybe something along the lines of:

message KeyValuePair {
  string key = 1;
  Expression value = 2;
}

message KeyValuePairList {
  repeated KeyValuePair elements = 1;
}

message Expression {
  oneof value {
    string string_value = 1;
    int64 int64_value = 2;
    bool bool_value = 3;
    List list_value = 4;
  }
}

message List {
  repeated Expression expressions = 1;
}

message StructuredAnnotation {
  string name = 1;
  oneof body {
    List list;
    KeyValuePairList kvpair_list = 2;
  }
}

This also means the invariants become much simpler, as there cannot be empty keys.

[chrispsommers]

@stefanheule Thanks for the fast review, that's a nice suggestion. I'm open to anything reasonable. Let's get more feedback, I'm happy to refactor to the best consensus solution.

[stefanheule]

Actually, thinking about it, it is not clear to me from the spec if constant list expressions are allowed or not. If they are not, then the protos can be even more simplified. If they can, the spec should probably be updated to say that. Right now it says:

All expressions (and nested expressions) within a stucturedAnnotationBody
must evaluate at compile-time to string literals, infinite-precision integers or
booleans.

That, to me, says there are no lists? But I'm not sure what the "nested expressions" means.

[chrispsommers]

@stefanheule "Nested expressions" was meant to include the case where an expression is an {expressionList}, to handle casaes like MyAnno[x=10,y={a,b,c}] - it might be very desirable to have the value of a kv-pair be a list of constant epressions.

[stefanheule]

I think that probably needs to be made more precise then. There are a number of nested expressions in P4, and most of them are not allowed (e.g. {kvlist}).

[chrispsommers]

@stefanheule Good feedback, Could you be so kind as to make that comment in the P4-Spec PR then? I welcome improvements to it. Thanks!

[stefanheule]

@stefanheule Good feedback, Could you be so kind as to make that comment in the P4-Spec PR then? I welcome improvements to it. Thanks!

Done.

[chrispsommers]

@stefanheule I tried out your proposed revised syntax against the examples I'd provided and indeed it is cleaner and more concise. Here is the a slightly-revised version of it, followed by the original example P4info and the revised P4info using the new syntax. For just these 4 simple examples, the revised P4info is 25 lines shorter (around 25% reduction). I'm in favor of changing to it, thanks for the suggestion. I'll wait a bit for more feedback, then proceed to update the PR accordingly.

Revised p4types.proto:

message KeyValuePair {
  string key = 1;
  Expression value = 2;
}

message KeyValuePairList {
  repeated KeyValuePair kv_pairs = 1;
}

message Expression {
  oneof value {
    string string_value = 1;
    int64 int64_value = 2;
    bool bool_value = 3;
    ExpressionList list_value = 4;
  }
}

message ExpressionList {
  repeated Expression expressions = 1;
}

message StructuredAnnotation {
  string name = 1;
  oneof body {
    ExpressionList expression_list = 2;
    KeyValuePairList kvpair_list = 3;
  }
}

Old P4Info for the examples:

=== Generate Empty structured anno ===
structured_annotations {
  name: "Empty"
}

=== Generate Structured expressionList ===
Generate expressionList ===
structured_annotations {
  name: "MixedExprList"
  elements {
    value {
      expression {
        int64_value: 1
      }
    }
  }
  elements {
    value {
      expression {
        string_value: "hello"
      }
    }
  }
  elements {
    value {
      expression {
        bool_value: true
      }
    }
  }
  elements {
    value {
      expression {
        bool_value: false
      }
    }
  }
  elements {
    value {
      expression {
        int64_value: 11
      }
    }
  }
}

=== Generate Structured kvList ===
structured_annotations {
  name: "Labels"
  elements {
    key: "short"
    value {
      expression {
        string_value: "Short Label"
      }
    }
  }
  elements {
    key: "hover"
    value {
      expression {
        string_value: "My Longer Table Label to appear in hover-help"
      }
    }
  }
}

=== Generate Complex kvList ===
structured_annotations {
  name: "MixedKV"
  elements {
    key: "label"
    value {
      expression {
        string_value: "text"
      }
    }
  }
  elements {
    key: "my_bool"
    value {
      expression {
        bool_value: true
      }
    }
  }
  elements {
    key: "list_key"
    value {
      expression_list {
        expressions {
          string_value: "a"
        }
        expressions {
          int64_value: 11
        }
        expressions {
          bool_value: true
        }
      }
    }
  }
  elements {
    key: "int_val"
    value {
      expression {
        int64_value: 6
      }
    }
  }
}

Revised P4Info:

=== Generate Empty structured anno ===
structured_annotations {
  name: "Empty"
}

=== Generate Mixed expressionList ===
structured_annotations {
  name: "MixedExprList"
  expression_list {
    expressions {
      int64_value: 1
    }
    expressions {
      string_value: "hello"
    }
    expressions {
      bool_value: true
    }
    expressions {
      bool_value: false
    }
    expressions {
      int64_value: 11
    }
  }
}

=== Generate kvList of  Strings===
structured_annotations {
  name: "Labels"
  kvpair_list {
    kv_pairs {
      key: "short"
      value {
        string_value: "Short Label"
      }
    }
    kv_pairs {
      key: "hover"
      value {
        string_value: "My Longer Table Label to appear in hover-help"
      }
    }
  }
}

=== Generate Complex kvList ===
structured_annotations {
  name: "MixedKV"
  kvpair_list {
    kv_pairs {
      key: "label"
      value {
        string_value: "text"
      }
    }
    kv_pairs {
      key: "my_bool"
      value {
        bool_value: true
      }
    }
    kv_pairs {
      key: "list_key"
      value {
        list_value {
          expressions {
            string_value: "a"
          }
          expressions {
            int64_value: 11
          }
          expressions {
            bool_value: true
          }
        }
      }
    }
    kv_pairs {
      key: "int_val"
      value {
        int64_value: 6
      }
    }
  }
}

[antoninbas]

I started reviewing this and then realized you had not updated the code with your latest changes yet.

So regarding the latest proposal in #265 (comment): it looks good to me. I see that for the sake of unifying the expression-list case and the KV case, we now allow nested expression lists in P4Info - which wasn't the case in your original proposal. Is the following also valid in P4?

@NestedExprLists[1,TEXT_CONST,true,1==2,{1, 2, {5, 6}}]

Based on p4lang/p4-spec#796 it looks like it is indeed legal, in which case it makes sense to support in P4Info. You may consider also including this example (or one like it) in the spec.

I didn't review the rest of the diff in details (I will after you update the code), but do you think it would be possible to add a note about integer expressions which cannot be represented with an int64? IIRC this is something we discussed at a WG meeting and the decision was that we would not support this in P4Info (and that the compiler would print an error suggesting the user to switch to a string instead).

[chrispsommers]

@stefanheule @antoninbas (and anyone else) please review simplified Expression in StructuredAnnotation, thanks. If this is satisfactory we will strive to get things approived in both the LDWG and the P4-API WG and start implementing. Thanks for all the engagement and feedback.

[antoninbas]

LGTM

If you can indulge me for one more thing, could you use P4Info instead of `P4Info` and p4info, in order to be consistent with the rest of the spec.

[chrispsommers]

I believe all issues discussed have been resolved in this PR and it is ready for merge.

[antoninbas]

LGTM

[antoninbas]

Alright, I think this PR has been open for long enough. Merging it now and we can make adjustments later if needed.

[chrispsommers]

@antoninbas Thanks, I was about to throw it a birthday party! :)

[chrispsommers]

BTW, the issues in p4-spec are settling down, I've clarified a number of things. There will need to be some minor changes in handling duplicate annotations.