Part 1 for PSA support #601

hanw · 2017-05-10T18:05:55Z

This is a rather large commit to refactor bmv2 backend to enable easier support for PSA.

A few major changes in this commit:

Separate JSON generation from IR manipulation. The jsonconverter.cpp had two jobs: traversing the IR and generating the corresponding JSON template. We introduced a new JsonObjects class that handles the JSON template generation and refactored jsonconverter.cpp into individual header.cpp, control.cpp, extern.cpp, action.cpp, parser.cpp, deparser.cpp files to limit each file to one aspect of the IR.
Separate V1Model-specific code generation routine. primitives are handled differently by v1model and PSA. We separated out the v1model-specific primitive generation into v1model.cpp to be backward-compatible with bm2-ss.
Preliminary extern generation for PSA.

As the code stands right now, it still generates valid JSON for bm2-ss. Our next step is to enable JSON generation for bm2-psa, and test with PSA behavior model.

In the interest of not keep a branch for too long, I hope we can fold this change in, to allow others to modify the refactored code, instead of adding changes to jsonconverter.cpp.

sethfowler · 2017-05-10T19:29:34Z

Yeah, we should get this reviewed and landed quickly, since it touches a huge number of files. It looks like you need to rebase already.

ChrisDodd · 2017-05-10T20:05:05Z

backends/bmv2/JsonObjects.cpp

+
+/// create a new field to an existing header type, returns a pointer to the field
+void
+JsonObjects::add_header_field(const cstring& name, Util::JsonArray** field) {


This function doesn't return anything, despite the comment. It also just uses *field, it doesn't try to modify it, so does it need the double pointer?

ChrisDodd · 2017-05-10T20:10:16Z

backends/bmv2/JsonObjects.h

+#ifndef _BACKENDS_BMV2_JSONOBJECTS_H_
+#define _BACKENDS_BMV2_JSONOBJECTS_H_
+
+namespace bm {


We generally use namespace BMV2 for bmv2 -- in any case we always use Capitalized names for namespaces.

ChrisDodd · 2017-05-10T20:28:22Z

backends/bmv2/control.cpp

+                    auto km = k->to<IR::Mask>();
+                    key->emplace("key", stringRepr(km->left->to<IR::Constant>()->value, k8));
+                    auto trailing_zeros = [](unsigned long n) { return n ? __builtin_ctzl(n) : 0; };
+                    auto count_ones = [](unsigned long n) { return n ? __builtin_popcountl(n) : 0;};


These functions should probably be added to lib/bitops.h rather than being defined inline here.

or rather lib/gmputil.h

ChrisDodd · 2017-05-10T20:36:11Z

backends/bmv2/deparser.h

+ public:
+    explicit ConvertDeparser(Backend* backend) {
+        passes.push_back(new DoDeparserBlockConversion(backend));
+        setName("ConvertDeparser");


Do you expect to want to add more passes here? If not, using a pass manager for a single pass is unnecessarily obscure.

ChrisDodd · 2017-05-10T20:47:23Z

backends/bmv2/lower.cpp

@@ -401,7 +401,8 @@ class ComplexExpression : public Inspector {
 const IR::PathExpression*
 RemoveComplexExpressions::createTemporary(const IR::Expression* expression) {
    auto type = typeMap->getType(expression, true);
-    auto name = refMap->newName("tmp");
+    LOG1("expression " << expression << " " << type);


LOG1 should only be used for messages that are self-contained and not too frequent, such as starting or ending a pass. This should probably be LOG3 or LOG4?

cc10512

There are a few things that I think you should remove from this PR, there are not ready for mainstream, and they are unlikely to conflict with future changes (e.g. the p4d2 tests, the remap for field aliases).

cc10512 · 2017-05-10T21:10:06Z

backends/bmv2/JsonObjects.cpp

+}
+
+/// append a json object r to a parent json array
+/// insert a field 'op' with 'name' to r


cc10512 · 2017-05-10T21:16:00Z

backends/bmv2/action.h

+
+class ConvertActions : public Inspector {
+    Backend*               backend;
+    P4::ReferenceMap*      refMap;


Is there a particular reason to have these as data members? You are initializing them from the backend data members.

cc10512 · 2017-05-10T21:17:03Z

backends/bmv2/backend.cpp

+
+namespace BMV2 {
+
+void Backend::createFieldAliases(const char *remapFile) {


This is not needed anymore, is it? We're not using remap files.

cc10512 · 2017-05-10T21:26:06Z

backends/bmv2/metadata.cpp

+*/
+
+/*
+Copyright 2013-present Barefoot Networks, Inc.


two copyright headers is one too many :)

cc10512 · 2017-05-10T21:26:48Z

backends/bmv2/metadata.cpp

+
+namespace BMV2 {
+
+}  // end namespace BMV2


this is not needed anymore.

cc10512 · 2017-05-10T21:28:28Z

backends/bmv2/metermap.cpp

+/**
+    Set the size of the table that a meter is attached to.
+
+    @param meter


unless you comment the parameters themselves, it's not needed to just add them as a @param.

cc10512 · 2017-05-10T21:29:55Z

backends/bmv2/metermap.h

+
+namespace BMV2 {
+
+class DirectMeterMap final {


class comment?

cc10512 · 2017-05-10T22:00:06Z

p4include/p4d2model_bmss_meta.map

@@ -0,0 +1,29 @@
+# This file maps p4d2 metadata to the bmv2 simple switch standard and intrinsic metadata


remove this file too.

cc10512 · 2017-05-10T22:01:51Z

testdata/p4_16_samples/ipv4_forward-bmv2.p4

@@ -0,0 +1,159 @@
+/* -*- P4_16 -*- */
+#include <core.p4>
+#include <p4d2model.p4>


is this test still failing? should we convert it back to v1model?

cc10512 · 2017-05-10T22:02:41Z

testdata/p4_16_samples/ipv4_forward-bmv2.stf

@@ -0,0 +1,8 @@
+set_default ipv4_lpm drop


before checking these in, we should make sure they are correct. I'm not convinced that the packet data is going to work for this test.

mihaibudiu

It will be impossible to review this patch in detail, given its size, and the fact that the diff tools do not work well across many files. I think that we will have to rely on the tests to a very large degree.

antoninbas

good work, hard to review thoroughly though

antoninbas · 2017-05-10T22:27:12Z

backends/bmv2/JsonObjects.h

+#define _BACKENDS_BMV2_JSONOBJECTS_H_
+
+namespace bm {
+


this header should probably include its dependencies, like <map>, etc and forward declare Util::JsonArray, etc.

by the way this is important to people not using a unified build

antoninbas · 2017-05-10T22:43:52Z

backends/bmv2/v1model.cpp

+            BUG_CHECK(hash->is<IR::Declaration_ID>(), "%1%: expected a member", hash);
+            auto algo = convertHashAlgorithm(hash->to<IR::Declaration_ID>()->name);
+            selector->emplace("algo", algo);
+            // FIXME selector_check pass


why was this commented-out?

good catch, I put it back.

antoninbas · 2017-05-10T22:47:33Z

backends/bmv2/action.cpp

+            if (type->is<IR::Type_StructLike>())
+                operation = "copy_header";
+            else
+                operation = "modify_field";


you should try to use the new bmv2 core primitives when possible given that they will be available on all archs: assign, assign_header... (see https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/core/primitives.h). For example assign is strictly equivalent to the "old" modify_field.

can you point me to the mapping for copy_header, add_header, remove_header and exit?

there is no mapping (also some new core primitives have no old equivalent), you'll have to go by the names
for exit, you should keep using exit, the name is staying the same

cc10512

Looks good.

- Comments on how to handle the semantics of multiple PRE invocations - and design option of splitting the PRE into PRE and BQE (Buffering and Queueing Engine) - added PRE as argument to Ingress and Egress - added missing 'in' qualifiers and random distribution - add size types instead of indefinite int - update with @mbudiu-vmw comments to pass through compiler - ActionProfile and ActionSelector externs

This commit refactors bmv2 backend to support multiple pipelien architectures. JsonConverter is refactored into a pass manager which is more conformed to the rest of the compiler design. Added files: backend.cpp - the new backend pass manager convert*.cpp - compiler passes for control, parser, deparser, externs and headers inferArchitecture - a new frontend pass to parse the architecture p4 into a data object which is then used by json generator model.cpp - a v2model that oriented for p4-16 IR TODO: tov1model - a pass to convert v2model to v1model to execute on simple-switch

Also fixes issues with decl handling and match types.

…y simple switch Allows access to bmv2 simple switch intrinsic and queueing metadata for programs that use the v1model. While these metadata would be available by defining separate metadata with the correct names, it is desirable that the functionality is exposed in the architecture definition. This patch will work with any metadata that has fields renamed.

mihaibudiu

Although the code has been merged, I think that some things need to be fixed. This review is not comprehensive, because it is very difficult to read code that has been split across files. It would have been much better if this was provided as a set of separate patches. This should also live in an experimental branch, and not in master, since it has not been reviewed properly.

mihaibudiu · 2017-05-11T00:16:22Z

Makefile.am

@@ -102,7 +102,8 @@ BUILT_SOURCES += \
 # FIXME -- and check-install-headers: below need to be consistent
 p4includedir = $(pkgdatadir)/p4include
 p4_14includedir = $(pkgdatadir)/p4_14include
-p4include_HEADERS += $(srcdir)/p4include/core.p4 $(srcdir)/p4include/v1model.p4
+p4include_HEADERS += $(wildcard $(srcdir)/p4include/*.p4) \
+		     $(wildcard $(srcdir)/p4include/*.map)


map files are something new; they deserve an explanation. They are also installed, so they seem quite important.

mihaibudiu · 2017-05-11T00:17:09Z

backends/bmv2/JsonObjects.cpp

+    field_aliases = insert_array_field(toplevel, "field_aliases");
+}
+
+/// insert a json array to a parent object under key 'name'


I think that we want phrase capitalization for such comments: start with an uppercase and end with a period.

mihaibudiu · 2017-05-11T00:18:28Z

backends/bmv2/JsonObjects.cpp

+void
+JsonObjects::add_meta_info() {
+    auto info = new Util::JsonObject();
+    static constexpr int version_major = 2;


This is actually the dialect version of the JSON language supported. I think that these constants should be in a more prominent place, not in code.

mihaibudiu · 2017-05-11T00:19:28Z

backends/bmv2/JsonObjects.cpp

+}
+
+/**
+ * create a header type in json


This function and the next ones should also have a @returns tag.

mihaibudiu · 2017-05-11T00:19:49Z

backends/bmv2/JsonObjects.cpp

+    return id;
+}
+
+/// create a heder type with empty field list.


typo in comment

mihaibudiu · 2017-05-11T18:52:06Z

p4include/p4d2model.p4

+
+/* P4-16 declaration of the P4-D2 (P4 Developer's Day) switch model */
+
+#include "core.p4"


This should be #include <core.p4>

mihaibudiu · 2017-05-11T18:53:02Z

p4include/v1model.p4

@@ -38,6 +38,18 @@ struct standard_metadata_t {
    bit<1>  drop;
    bit<16> recirculate_port;
    bit<32> packet_length;
+    // flattening fields that exist in bmv2-ss
+    // queueing metadata
+    @alias("queueing_metadata.enq_timestamp") bit<32> enq_timestamp;


The @alias annotation is not documented. What does it mean?

@alias is used to generate the field_alias section of the BMV2 JSON. Field alias creates a mapping from the metadata name in P4 program to the behavioral model's internal metadata name. Here we use it to expose all metadata supported by behavior model to user through standard metadata, as requested by @vgurevich

You have to add this description to the v1model.p4 file itself.

mihaibudiu · 2017-05-11T18:54:12Z

p4include/v1model.p4

@@ -133,14 +145,17 @@ parser Parser<H, M>(packet_in b,
                    inout standard_metadata_t standard_metadata);
 control VerifyChecksum<H, M>(in H hdr,
                             inout M meta);
+@pipeline


The @pipeline annotation is not documented anywhere. I think it should be removed.
You are essentially duplicating functionality provided by the package operation - the fact that something is a pipeline is given by the fact that it is used as an argument to the V1Switch package in the right position.

The @pipeline annotation is needed because BMv2 JSON mandates only ingress and egress can be in the pipeline section. The original BMv2 backend hard-coded the name of ingress and egress in v1model when generating the pipeline section in JSON. When I wrote the visitor, I did not figure out a way to traverse only ingress and egress, and not verifychecksum and updatechecksum without hard-coding the names. They are all Controlblock from the visitor's point of view.

The annotation is on the type, not on the instance, so this way of doing it won't work.
For example, you could very well have multiple blocks with the same type required in the package, e.g., the same signature for checksum updates and for the main pipeline.

The way you distinguish them is by the argument of package they bind to. That's why there is a pass which takes a policy about which controls to skip. What you have to do is something similar for the P2D2 architecture: a list of controls to skip.

Previously, the skipControls policy receives the list of control blocks to skip from the hard-coded names in v1model.

p4c/backends/bmv2/midend.cpp

Line 126 in b25ba2d

auto ingress = main->getParameterValue(P4V1::V1Model::instance.sw.ingress.name);

I was looking for a way to generalize how skipControls (and other policies) computes the list of control blocks. The solution I came up with was to use special annotations in the architecture file. That way, at least I don't have to change the backend code for a different architecture. By putting the annotation on the type, only the person who designs the architecture needs to care about this detail, and it's not visible to the P4 programmer. It also affects the checksum passes, as I need a way to identify which control block is updateChecksum. It would be argument 5 in v1model or argument n ( n != 5) in PSA, It may not have the name updateChecksum in PSA, or other architecture that people come up with. I am not sure how to identify updateChecksum without using annotation?

I understand what you are trying to do.
The problem comes from the fact that you are trying to support multiple architectures with a single back-end. That will be harder and harder as these architecture diverge in behaviors. For example, changes you make for PSA may break v1model programs. Also, the BMv2 back-end generates a program called p4c-bm2-ss, so you see that it targets a specific simulator on top of BMv2 (bmv2 can be used to implements lots of different simulators), and it it assumes a specific model for the input architecture, v1model. These things are tightly coupled in the code; the C++ code hardwires a lot of knowledge about v1model and BMv2 SS in particular. You are trying to de-entangle them, but that won't be easy.

However, v1model should be thought as a description of a fixed architecture, which cannot be changed. In general, you cannot assume you can edit it anymore after you have distributed it to customers.

Moreover, regarding the @pipeline annotations, all the information you need IS in the package. It is just annoying to extract. Maybe you can write some helper functions to make this easier. For v1model we know that the pipelines are arguments 3 and 4 of the main package, and the deparser is argument 6 (the code in fact uses the parameter names in v1model to retrieve them). This is a contract between the architecture and the compiler, and it can never change once you published the architecture, so you can rely on it. Similarly, for PSA you will identify the pipelines from the arguments to the main package. So what you have to do is write a generic class that can identify the blocks from the main package, provide two implementations for PSA and v1model and output the same data-structure that you can use from both. I would suggest not using annotations, but just set, the same way it was done in the BMv2 back-end.

mihaibudiu · 2017-05-11T18:55:22Z

p4include/psa.p4

+ * These types need to be defined before including the architecture file
+ * and the macro protecting them should be defined.
+ */
+#ifndef PSA_CORE_TYPES


Is this file the most recent version?
It looks like this file cannot compile.

We haven't fully tested psa.p4 yet. I will do it in the next PR.

Maybe you can then remove it from the PR?

mihaibudiu · 2017-05-11T18:55:39Z

p4include/psa.p4

+/**
+ * PSA supported metadata types
+ */
+struct psa_parser_input_metadata_t {


indentation is wrong.

hanw · 2017-05-11T19:12:47Z

Thanks very much for the review, Mihai. I will address your comments today.

hanw requested review from mihaibudiu, ChrisDodd and cc10512 May 10, 2017 18:06

ChrisDodd approved these changes May 10, 2017

View reviewed changes

cc10512 approved these changes May 10, 2017

View reviewed changes

mihaibudiu reviewed May 10, 2017

View reviewed changes

antoninbas reviewed May 10, 2017

View reviewed changes

cc10512 approved these changes May 10, 2017

View reviewed changes

Calin Cascaval and others added 21 commits May 10, 2017 16:57

adds metadata remapping (#4)

13e2b15

fix gtest

16c32aa

simplified filenames

7e345e9

factor out extern json generation from jsonconverter to v1model (#5)

2de4e05

V1Model::convertExternObjects

a484540

adds extern function serialization (#6)

bbd4e5d

checkpoint

584c61b

add midend pass to transform extern method, simplify v2model

260b313

fix error code generation

e129eab

use v1model header generation routine

6a01520

add headerstack generation

f5bf970

support for directCounterMap (#7)

87934f2

refactor deparser

b71aab7

fixed issues with pipeline generation

d441c26

start to add metadata handling (#8)

ff285d6

Also fixes issues with decl handling and match types.

fix action ids and access to architecture annotations

95ec081

fix standard metadata generation

889a7a8

fix deparser generation

63012bb

clean up

c651b1f

Han Wang and others added 19 commits May 10, 2017 17:01

clean up header and parser generation

6b3cb58

simplified header generation code

03a94d3

tweaks p4d2model and adds testcase (#13)

3c44059

moved actions outside control to macros (#14)

9fd2f6a

bugfix in enum generation

6a6fd9b

add ipv4_forward example for p4d2 (#15)

d57aaaa

refactor done on header.cpp

beb43f6

clean up

9af4581

clear warnings

c8e00df

use conv instead of getExpressionConverter

59fde68

clean up extern generation

95fd63e

minor

6cb3b07

fix duplicated standard_metadata bug

7c59a80

change @name annotation to @alias to avoid generating name in json

7bf1a65

revert changes to lib/error.h and parser4.p4

7916c99

address review comments

178b0f9

remove unused test case

4bab1e4

put back actionSelectorCheck in control.cpp

0e4283d

hanw force-pushed the hanw/psa branch from 5bcf930 to 0e4283d Compare May 11, 2017 00:02

Han Wang added 3 commits May 10, 2017 17:24

fixing rebase errors

90965fa

fix an error in inferArchitecture.cpp, update drop-bmv2 sample output

c08bec7

cpplint

fd729d4

hanw merged commit 0623eaa into p4lang:master May 11, 2017

mihaibudiu reviewed May 11, 2017

View reviewed changes

hanw deleted the hanw/psa branch May 11, 2017 22:15

hanw restored the hanw/psa branch May 11, 2017 23:54

hanw deleted the hanw/psa branch May 12, 2017 00:14

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed


		namespace BMV2 {

		void Backend::createFieldAliases(const char *remapFile) {

		@@ -0,0 +1,29 @@
		# This file maps p4d2 metadata to the bmv2 simple switch standard and intrinsic metadata


		/* P4-16 declaration of the P4-D2 (P4 Developer's Day) switch model */

		#include "core.p4"

Part 1 for PSA support #601

Part 1 for PSA support #601

Conversation

hanw commented May 10, 2017

sethfowler commented May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc10512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc10512 left a comment

Choose a reason for hiding this comment

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanw commented May 11, 2017

sethfowler commented May 10, 2017 •

edited

Loading