Implement support for non-const table entries #3748

mihaibudiu · 2022-12-01T18:34:18Z

Signed-off-by: Mihai Budiu mbudiu@vmware.com

This validates the design in p4lang/p4-spec#1180

mihaibudiu · 2022-12-01T18:35:49Z

This implementation does not have the code to generate the priorities yet, but validates the grammar

mihaibudiu · 2022-12-01T18:41:33Z

there are a bunch of failures due to the inability to install the software in CI.
I don't understand the bazel complaints:

ERROR: /home/runner/work/p4c/p4c/BUILD.bazel:180:11: Compiling frontends/common/constantFolding.cpp failed: undeclared inclusion(s) in rule '//:ir_frontend_midend_control_plane':
this rule is missing dependency declarations for the following files included by 'frontends/common/constantFolding.cpp':
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/stddef.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/stdarg.h'

I haven't touched this file.

mihaibudiu · 2022-12-01T18:42:01Z

@smolkaj any clues why bazel is unhappy?

smolkaj · 2022-12-01T18:47:59Z

This is the error message:

ERROR: /home/runner/work/p4c/p4c/BUILD.bazel:180:11: Compiling frontends/common/constantFolding.cpp failed: undeclared inclusion(s) in rule '//:ir_frontend_midend_control_plane':
this rule is missing dependency declarations for the following files included by 'frontends/common/constantFolding.cpp':
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/stddef.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/stdarg.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/stdint.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/limits.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/syslimits.h'
  '/usr/lib/gcc/x86_64-linux-gnu/11/include/float.h'
...

Did you make any changes to frontends/common/constantFolding.cpp? Not as far as I can tell?

mihaibudiu · 2022-12-01T18:48:58Z

I didn't that's why I find this puzzling.

mihaibudiu · 2022-12-01T19:06:36Z

@smolkaj Some people say it's a caching bug: https://stackoverflow.com/questions/53527134/bazel-missing-dependency-declarations-for-stddef-h-and-others-ubuntu-18-04

smolkaj · 2022-12-01T21:03:10Z

You could try clearing the cache here? https://github.com/p4lang/p4c/actions/caches?query=key%3ALinux-build+

EDIT: source: https://stackoverflow.com/questions/63521430/clear-cache-in-github-actions

smolkaj · 2022-12-02T03:53:30Z

I tried on my laptop and couldn't reproduce the issue. So could indeed be a caching issue. If we can't figure out how to clear the cache, here are some random things we might try...:

add a .bazelrc with the following contents to the root of the repo:

# Use C++17.
build: --cxxopt=-std=c++17 --host_cxxopt=-std=c++17

replace ubuntu-latest with ubuntu-22.04 in ci-bazel.yml

EDIT: An easy workaround that will clear the cache: in ci-bazel.yml, replace

        key: ${{ runner.os }}-build-direct-${{ github.sha }}

with something like

        key: bazel-${{ runner.os }}-build-direct-${{ github.sha }}

fruffy · 2022-12-02T18:04:24Z

I cleared the cache. Looks like it is fixed.

fruffy · 2022-12-05T21:37:47Z

testdata/p4_16_samples_outputs/entries-prio.p4

+}
+
+control c(in Headers h) {
+    action a() {


It would be good if those actions have some effect on outputs to test the compiler passes.

mihaibudiu · 2023-03-03T18:23:17Z

@jafingerhut can you please review this?

mihaibudiu · 2023-04-14T18:10:01Z

@jafingerhut this now uses the syntax with mandatory 'priority' and a colon after the priority

mihaibudiu · 2023-04-14T18:10:27Z

If we settle on this syntax I will implement the additional features in the spec PR, e.g., priority assignment.

mihaibudiu · 2023-05-19T20:10:14Z

@jafingerhut the last commit is meant to implement the spec completely with regards to non-const entries

jafingerhut · 2023-05-19T20:37:01Z

@mihaibudiu Thanks! I will try to review at least the test cases, and perhaps parts of the implementation regarding generation of warnings, soon-ish.

mihaibudiu · 2023-05-19T20:38:01Z

I have tested the warning generation too, but was too lazy to add the tests.

jafingerhut · 2023-05-19T20:47:31Z

@mihaibudiu Fortunately adding test cases and checking their outputs is my p4c specialty! :-)

jnfoster · 2023-05-19T20:56:19Z

Human p4testgen ;-)

…

On Fri, May 19, 2023 at 4:47 PM Andy Fingerhut ***@***.***> wrote: @mihaibudiu <https://github.com/mihaibudiu> Fortunately adding test cases and checking their outputs is my p4c specialty! :-) — Reply to this email directly, view it on GitHub <#3748 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOCFUS4TAESGXSFZYBP2PTXG7L65ANCNFSM6AAAAAASRC44VA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

jafingerhut · 2023-05-20T17:11:21Z

Thanks for this implementation. I noticed that it is a bit different in the way it assigns priorities to entries than the language spec proposes.

The spec says that if some priorities are explicitly given by the P4 developer in the source code, but not all, that

(1) the first entry must have a priority explicitly specified.
(2) pseudocode in the spec explains how all other entry priorities are determined, from earliest entries in the source to the last entry in the source.

This PR as of commit 4 has a different implementation, where if largest_priority_wins is true, it assigns priorities from last entry in the source back to the first.

I think it would be good for the spec and implementation to match in how priorities are assigned to entries. Either we should review a PR that updates the spec to match this implementation, or we should make this implementation match the spec. My personal preference would be to change this implementation to match the spec.

jafingerhut · 2023-05-20T18:17:49Z

Another thing that would be good to have: in the code that determines the entry priorities, save the results of the determined priority values in such a way that the control plane API generation code can read and output those priority values.

I understand that if you want to emit P4_16 source code from the IR after the priorities are determined, in such a way that only the entries that the P4 developer explicitly specified priority values will have them in that printed P4_16 source code later, you would need a way in the IR to remember which entries have priority values from the source code, versus which ones have priority values determined by the compiler.

But another alternative is to explicitly choose that the P4_16 code emitted from the IR does have explicit priority values for every entry. I cannot think of any harm if we do that. Not only would there be no harm, but it would be a way to verify that the compiler-determined priority values were correct, by diff'ing them against the priority values in the -frontend.p4 and/or -midend.p4 files output by the compiler, and compared against files stored in this repo.

jafingerhut · 2023-05-20T18:42:16Z

frontends/p4/entryPriorities.cpp

+    CHECK_NULL(table);
+    auto ep = table->properties->getProperty(IR::TableProperties::entriesPropertyName);
+    CHECK_NULL(ep);
+    if (ep->isConstant) return entries;


I think that if method tableNeedsPriority() returns false for this table, that we should not execute the code below that calculates entry priorities. Instead, we should issue a warning or error if the source code contains explicit priority values for any entry.

If the P4 developer tries to specify priority values for such a table, I think they have a fundamental misunderstanding, and should get an error that says something like "This table cannot support entry priorities. For example, tables with only exact and lpm match kinds cannot support entry priorities".

TODO Andy: Create a test program to exercise this case, and expect an error message.

Inside the zip file I have attached to this comment: #3748 (comment) the test program init-entries-error1-bmv2.p4 contains 3 different but similar table definitions that I believe should all give an error message at compile time for the reason mentioned in my comment above.

jafingerhut · 2023-05-21T01:33:32Z

I have attached a zip file to this comment containing 4 new test programs for this issue. Each has one or more table definitions in it, with comments explaining what warnings or errors I would expect the compiler to issue for it. The ones in p4_16_errors currently cause no errors to be issued with the changes in this PR as of commit 4.

p4c-pull-3748-test-progs.zip

mihaibudiu · 2023-05-22T16:24:33Z

Another thing that would be good to have: in the code that determines the entry priorities, save the results of the determined priority values in such a way that the control plane API generation code can read and output those priority values.

This is what we are doing: after this code every entry in a non-const entries table will have a priority.

fruffy · 2023-05-26T17:13:24Z

I would just add p4tools_add_xfail_reason( "testgen-p4c-bmv2-ptf" "Error when adding match entry to target" init-entries-bmv2.p4 ) to the BMV2PTFXfail.cmake file.

fruffy · 2023-05-26T17:28:44Z

backends/p4tools/modules/testgen/targets/bmv2/test/''

@@ -0,0 +1,290 @@
+# XFAILS: tests that currently fail. Most of these are temporary.


I think something went wrong with the file here.

jafingerhut · 2023-06-01T20:58:40Z

Is it clear to anyone what might be causing the macOS test failures?

fruffy · 2023-06-01T23:29:57Z

2023-05-26T18:21:40.7270690Z /Users/runner/work/p4c/p4c/frontends/p4/entryPriorities.cpp:77:33: error: call to constructor of 'IR::Constant' is ambiguous
2023-05-26T18:21:40.7276490Z             auto priority = new IR::Constant(currentPriority);

mihaibudiu · 2023-06-01T23:31:08Z

I introduced this bug, but the "pedantic" flag for MacOS produces such huge logs that I could never find it.

fruffy · 2023-06-01T23:31:41Z

Also recommend adding this:
https://github.com/p4lang/p4c/pull/3970/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR241
to get rid of the excessive warnings, it should help.

fruffy · 2023-06-07T18:17:32Z

@mihaibudiu I merged a PR that cleans up the MacOS error message. I believe there is still an error of the same kind left.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

jafingerhut · 2023-06-07T22:48:46Z

Yay! All tests passing, 2 positive reviews. Time to merge? I am happy to do it.

mihaibudiu requested a review from jafingerhut December 1, 2022 18:34

smolkaj mentioned this pull request Dec 1, 2022

this rule is missing dependency declarations for the following files included by 'external/com_google_protobuf/src/google/protobuf/util/internal/json_escaping.cc' bazelbuild/bazel#5164

Closed

mihaibudiu force-pushed the prio branch from 6f25a2d to 84fdca9 Compare December 1, 2022 19:07

fruffy reviewed Dec 5, 2022

View reviewed changes

jafingerhut mentioned this pull request Apr 11, 2023

Define "initial entries" for P4 tables, in addition to currently defined "const entries" p4lang/p4-spec#1159

Closed

10 tasks

mihaibudiu force-pushed the prio branch from e77e125 to 30daa58 Compare April 14, 2023 18:09

mihaibudiu force-pushed the prio branch from 30daa58 to 8e44ddb Compare May 19, 2023 20:09

jafingerhut reviewed May 20, 2023

View reviewed changes

mihaibudiu force-pushed the prio branch from 8e44ddb to 9ed35c2 Compare May 22, 2023 17:15

mihaibudiu force-pushed the prio branch from 25d73f6 to b01436c Compare May 26, 2023 17:14

fruffy reviewed May 26, 2023

View reviewed changes

mihaibudiu force-pushed the prio branch from b01436c to b880e96 Compare May 26, 2023 18:08

fruffy approved these changes May 26, 2023

View reviewed changes

jafingerhut mentioned this pull request May 30, 2023

Changes to grammar in spec to make it closer to the p4c implementation p4lang/p4-spec#1256

Merged

mihaibudiu force-pushed the prio branch from b880e96 to 7783d34 Compare June 1, 2023 23:34

Mihai Budiu and others added 12 commits June 7, 2023 11:21

Implement support for non-const table entries

56641f4

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>

Changed reference outputs

7569f13

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>

Add colon after priority

ead812d

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>

Priority assignment

a97f31e

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

Test cases for priority assignment

85b199e

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

Add missing include

c4c8d0a

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

P4Runtime should use entry priorities if they exist

52f56fa

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

Fix inverted condition

a2de868

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

Fix reference outputs

dd5c11b

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

xfail the Testgen for non-const entries

9dbe493

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

Improve ambiguous constructor call

aa86973

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

MacOS build fix

4d1607d

Signed-off-by: Mihai Budiu <mbudiu@gmail.com>

mihaibudiu force-pushed the prio branch from 7783d34 to 4d1607d Compare June 7, 2023 18:44

mihaibudiu merged commit b80b2cb into p4lang:main Jun 7, 2023

mihaibudiu deleted the prio branch June 7, 2023 22:50

fruffy mentioned this pull request Jun 12, 2023

Unexpected PRIORITY error message #4026

Closed

jafingerhut mentioned this pull request Jun 13, 2023

Minor fix-ups to support for tables with initial entries and their P4Runtime API output files #4016

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for non-const table entries #3748

Implement support for non-const table entries #3748

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

smolkaj commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

smolkaj commented Dec 1, 2022 •

edited

Loading

smolkaj commented Dec 2, 2022 •

edited

Loading

fruffy commented Dec 2, 2022

fruffy Dec 5, 2022

mihaibudiu commented Mar 3, 2023

mihaibudiu commented Apr 14, 2023

mihaibudiu commented Apr 14, 2023

mihaibudiu commented May 19, 2023

jafingerhut commented May 19, 2023

mihaibudiu commented May 19, 2023

jafingerhut commented May 19, 2023

jnfoster commented May 19, 2023 via email

jafingerhut commented May 20, 2023 •

edited

Loading

jafingerhut commented May 20, 2023 •

edited

Loading

jafingerhut May 20, 2023 •

edited

Loading

jafingerhut May 21, 2023

jafingerhut commented May 21, 2023

mihaibudiu commented May 22, 2023

fruffy commented May 26, 2023 via email •

edited

Loading

fruffy May 26, 2023

jafingerhut commented Jun 1, 2023

fruffy commented Jun 1, 2023

mihaibudiu commented Jun 1, 2023

fruffy commented Jun 1, 2023

fruffy commented Jun 7, 2023

jafingerhut commented Jun 7, 2023

		@@ -0,0 +1,290 @@
		# XFAILS: tests that currently fail. Most of these are temporary.

Implement support for non-const table entries #3748

Implement support for non-const table entries #3748

Conversation

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

smolkaj commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

mihaibudiu commented Dec 1, 2022

smolkaj commented Dec 1, 2022 • edited Loading

smolkaj commented Dec 2, 2022 • edited Loading

fruffy commented Dec 2, 2022

fruffy Dec 5, 2022

Choose a reason for hiding this comment

mihaibudiu commented Mar 3, 2023

mihaibudiu commented Apr 14, 2023

mihaibudiu commented Apr 14, 2023

mihaibudiu commented May 19, 2023

jafingerhut commented May 19, 2023

mihaibudiu commented May 19, 2023

jafingerhut commented May 19, 2023

jnfoster commented May 19, 2023 via email

jafingerhut commented May 20, 2023 • edited Loading

jafingerhut commented May 20, 2023 • edited Loading

jafingerhut May 20, 2023 • edited Loading

Choose a reason for hiding this comment

jafingerhut May 21, 2023

Choose a reason for hiding this comment

jafingerhut commented May 21, 2023

mihaibudiu commented May 22, 2023

fruffy commented May 26, 2023 via email • edited Loading

fruffy May 26, 2023

Choose a reason for hiding this comment

jafingerhut commented Jun 1, 2023

fruffy commented Jun 1, 2023

mihaibudiu commented Jun 1, 2023

fruffy commented Jun 1, 2023

fruffy commented Jun 7, 2023

jafingerhut commented Jun 7, 2023

smolkaj commented Dec 1, 2022 •

edited

Loading

smolkaj commented Dec 2, 2022 •

edited

Loading

jafingerhut commented May 20, 2023 •

edited

Loading

jafingerhut commented May 20, 2023 •

edited

Loading

jafingerhut May 20, 2023 •

edited

Loading

fruffy commented May 26, 2023 via email •

edited

Loading