-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rebased version of #1698 #2902
Rebased version of #1698 #2902
Conversation
Awesome, thanks. I agree that we would want to instead add new functions and deprecate the old ones. EDIT: Or |
db3b0da
to
29d8125
Compare
@smolkaj can you please test to see whether this variant works for you? |
Thanks, @mbudiu-vmw, I will give it a try! |
29d8125
to
94a3cda
Compare
Question, how can one specify not passing any fields to recirculate/resubmit/clone3. E.g., the equivalent of |
Yes, just use a number that has no fields attached. You could use -1, that's less likely to be a common number. |
Thanks for the response @mbudiu-vmw. I still feel this can lead to subtle bugs but @smolkaj mentioned that there is the |
p4include/v1model.p4
Outdated
* calling clone3 with the same type and session parameter values, | ||
* with empty data. | ||
*/ | ||
extern void recirculate_field_list(bit<8> index); | ||
extern void clone(in CloneType type, in bit<32> session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another declaration of extern void clone(in CloneType type, in bit<32> session)
in line 537. This causes compilation errors:
[--Werror=duplicate] error: clone: multiple matching declarations
...
v1model.p4(537): [--Werror=duplicate] error: Candidate: clone
extern void clone(in CloneType type, in bit<32> session);
^^^^^
v1model.p4(586): [--Werror=duplicate] error: Candidate: clone
extern void clone(in CloneType type, in bit<32> session);
^^^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come our tests pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what flags/options we are using for the compilation (maybe @smolkaj can shed some light?). Anyway that declaration is indeed duplicated in v1model.p4 in this pull request (lines 537 and 586).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if you never call the method you don't get the error.
I will remove one of the two declarations, in the meantime can you please do the same if you want to try this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, also maybe somebody should add a test for clone
to p4c's test suite to avoid future regressions. Yeah I commented out that line and the error goes away.
One more question, I have something like:
I get the error:
However, if I change
It works fine (at least in compilation). I was wondering whether I'm missing something here. |
This is a bit strange, because this exactly the kind of code that is generated by the P4-14 front-end. |
I randomly selected |
If you look at this web page you will see that all tests have passed. I will give it a shot. |
Sorry if this is not the right place for this kind of question but I'd like to get a sneak peek of this version of p4c, with the extern |
Can you already build the main branch of the compiler? |
Don't use my clone of the repository, use the official one from https://github.com/p4lang/p4c |
Great. It worked. Thanks @mbudiu-vmw |
@mbudiu-vmw @smolkaj |
+1, how about |
(As you might be able to tell, Google has trained me to advocate for long, descriptive function names... 😆) |
I will submit soon an update which fixes the issues you discussed. There is a new test using an enum, and that passes too, I could not reproduce Ali's error. Make sure that the enum is declared prior to its first use. |
…g#2971) * Fix bug in file def_use in method setDefinitionsAt class AllDefinitions
@mbudiu-vmw Thanks for the changes. One question regarding the naming: what does |
It comes from the fact that there was a clone with 2 arguments, and one with 3. |
* Treat error type as string in p4RuntimeSerializer
@mbudiu-vmw Thanks for the info. nit: now that this action is completely distinguishable from the clone with 2 arguments, would it make sense to rename it to |
I will make the change to clone3, but we won't be able to merge this until someone approves it. |
/* Treat error type as string */ | ||
if (type->is<IR::Type_Error>()) | ||
return 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not reviewing the C++ implementation code parts of this PR very carefully -- simply scanning through them looking for anything odd. Is this change something that was discovered as a small side issue while developing the fixes for preserving user-defined metadata fields for resubmit/recirculate/clone? Perhaps something that was needed for the failing test cases exercising those features to pass, so good to combine them into this PR while you were at it? I don't have any objection, main curious why it is thrown in with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is coming from the main branch from a rebase.
@@ -2588,9 +2675,12 @@ void ProgramStructure::populateOutputNames() { | |||
"mark_to_drop", | |||
"hash", | |||
"resubmit", | |||
"resubmit_preserving_field_list", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will double-check this with my own small tests, but is resubmit_preserving_field_list a new primitive internal to p4c only, or do these changes also require new primitives in the BMv2 JSON file that BMv2 reads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is compiled to the standard recirculate bmv2 primitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thanks.
* @field_list(1, 2) | ||
* bit<32> y; | ||
* bit<32> z; | ||
* } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a short example to this documentation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a separate issue to remind me to either myself, or find some other volunteer to help, update other documentation inside and outside of the p4lang/p4c repository on how these operations work. I think it is best of those changes are made in their own separate PRs after this one is merged: #2977
@@ -134,16 +134,27 @@ header data_t { | |||
} | |||
|
|||
struct metadata { | |||
@pa_solitary("ingress", "acl_metadata.if_label") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How were tests passing before without these annotations? The input program had a pragma or P4_14 annotation-like thing since before this PR, so did something change in this PR to make these annotations appear in the output? Or were the tests somehow passing before even though the files did not match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably because I rebased on top of other changes. This pr does not know anything about this annotation. I don't know where it's coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will not worry about it then. There is a pa_solitary
pragma in the original P4_14 program, so something in the P4_14 to P4_16 translation must be picking it up.
@@ -73,7 +77,7 @@ control egress(inout headers hdr, inout metadata meta, inout standard_metadata_t | |||
|
|||
control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) { | |||
@name(".do_copy_to_cpu") action do_copy_to_cpu() { | |||
clone3<tuple<standard_metadata_t>>(CloneType.I2E, 32w250, { standard_metadata }); | |||
clone_preserving_field_list(CloneType.I2E, 32w250, (bit<8>)FieldLists.copy_to_cpu_fields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this implementation work to preserve standard metadata fields? If yes, how does that work, if one cannot add annotations to the standard metadata struct definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the answer, it seems worth adding a sentence or three on this in the v1model.p4 comments that document how this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot. The idea is that you should not be able to spoof standard Metadata. It's debatable, but I don't know how to do it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable restriction, and I will put it on a list of details to document elsewhere, including a suggested workaround that of course one is welcome to copy a standard metadata field value into a user-defined metadata field, and preserve the user-defined metadata field.
// Copy another header's data to local variable. | ||
bh.x = h.bvh0.x; | ||
clone3(CloneType.I2E, 0, h); | ||
clone_preserving_field_list(CloneType.I2E, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like if we wanted to remain faithful to the original meaning of this test program, there should be @field_list(0)
annotations on all fields of struct parsed_packet_t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone only preserves metadata fields,. Adding annotations to parsed_packet_t should have no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. As usual, I have focused on the v1model.p4 include file changes, and the changes to the test programs and their expected outputs. Except for one minor comment where it appears that a test program may have changed its behavior by no longer preserving some fields that the original did, I did not notice any problems.
@jafingerhut if I have your approval please give it officially so we can merge this. |
jafingerh is also my user account, and I approved using that one already. I can add approval using my account jafingerhut in a few minutes, in case that makes a difference for tracking purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. As usual, I have focused on the v1model.p4 include file changes, and the changes to the test programs and their expected outputs. Except for one minor comment where it appears that a test program may have changed its behavior by no longer preserving some fields that the original did, I did not notice any problems.
…nges In particular after this pull request was merged into p4c: p4lang/p4c#2902
#1057) * Updates to documentation on metdata preservation after recent p4c changes In particular after this pull request was merged into p4c: p4lang/p4c#2902 * Remove some unnecessary changes from first commit Should make this easier to review. * Fix minor typo * Update the v1model packet_length documentation based on code changes done on 2021-Dec-16.
This is another attempt to fix the problems with recirculate/resubmit/clone3.
The solution is to
@field_list
currently) on the user metadata fields that need to be recirculatedThis will cause the runtime to initialize the specified fields before entering the pipeline.
This correctly implements the semantics of P4-14 recirculate/etc.
Here is an excerpt from the
resubmit.p4
p4-14 benchmark, as automatically translated by the compiler:This changes the signature of the respective v1model functions. We may chose instead to add new functions and deprecate the old ones.