Enable handling of arbitrary if statements within actions for BMv2 back end #4999
Conversation
…ck end Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
|
This eliminates the buggy Predication pass:
predication-pass
In its place, this PR handles arbitrarily nested if statements with arbitrary statements that can appear in action bodies: not only assignment statements, but extern function/method calls, return, and exit statemnts. It does this by taking advantage of support for jump and conditional jump primitive actions that were implemented in BMv2 in 2017! (Thanks, Antonin -- sorry the p4c back end support was not implemented for 7 years). See https://github.com/jafingerhut/p4-guide/blob/master/docs/if-statements.md#bmv2 for some details. |
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
|
This also fixes known failures in P4Testgen for STF and PTF: |
Signed-off-by: Andy Fingerhut <[email protected]>
|
Would this fix #644? One of the oldest open issues for the compiler. |
There was a problem hiding this comment.
LGTM, thanks for this @jafingerhut
yes, this will fix #644, which was the reason why I added the _jump and _jump_if_zero primitives to bmv2
backends/bmv2/common/action.cpp
Outdated
| if (!inConditional) { | ||
| // Similar to return statement optimization above. | ||
| break; | ||
| } | ||
| // Similar to return statement jump above. | ||
| unsigned int curOffset = result->size(); | ||
| auto parameters = new Util::JsonArray(); | ||
| auto primitive2 = mkPrimitive("_jump"_cs, result); | ||
| (*offsetToTargetLabelId)[curOffset] = labelIdEndOfAction; | ||
| (*offsetToJumpParams)[curOffset] = parameters; | ||
| primitive2->emplace_non_null("source_info"_cs, s->sourceInfoJsonObj()); | ||
| continue; |
There was a problem hiding this comment.
nice catch. One could argue that the bmv2 implementation is not ideal here: with the existence of a dedicated exit primitive, bmv2 could have interrupted the execution of the action. But at least this way things are symmetrical with return (I could have added a return primitive too...).
|
@fruffy Would you like any additional reviews on this, or is it OK to merge? |
There was a problem hiding this comment.
Tagging @smolkaj and @jonathan-dilorenzo as stakeholders. From my side this seems good-to-go.
@r12f this might be useful for your DASH modelling efforts?
| @@ -25,7 +25,12 @@ namespace P4::BMV2 { | |||
| class ActionConverter : public Inspector { | |||
| ConversionContext *ctxt; | |||
|
|
|||
| void convertActionBody(const IR::Vector<IR::StatOrDecl> *body, Util::JsonArray *result); | |||
| void convertActionBodyTop(const IR::Vector<IR::StatOrDecl> *body, Util::JsonArray *result); | |||
| void convertActionBody(const IR::Vector<IR::StatOrDecl> *body, Util::JsonArray *result, | |||
There was a problem hiding this comment.
Nit: I'd consider making this a struct that has all this information. Makes it easier to change and read.
There was a problem hiding this comment.
Good idea. Done in commit 7.
Jonathan and Steffen -- I believe these changes are completely backwards compatible, meaning that if a P4 program for BMv2 v1model compiles and runs correctly today, it should continue to do so with these changes. Call p4c-bm2-ss without these changes "A", and with these changes "B". There exist P4 programs with if statements in actions that give no compile-time error with A, but generate BMv2 JSON files with bugs such that when they process packets, they behave differently than the P4 program specifies. These are fairly rare -- I believe you must have nested if statements within an action body to trigger these bugs in version A, but they do exist. Version B should fix all such bugs. There also exist P4 programs with if statements in actions that give compile-time errors with A, and generate no BMv2 JSON file. Such programs should compile without error with version B, and produce BMv2 JSON files that behave as specified by the P4 program. |
Signed-off-by: Andy Fingerhut <[email protected]>
Woot!! Absolutely! Thanks a lot Fabian and Andy! Finally can remove the tricky ternary operators now! |
|
The only failing test is for compiling Tofono back end, which appears to be related to issues between not being able to recognize string listerals as type cstring, or something similar to that, unrelated to these changes. |
|
Awesome! I have definitely wanted this in the past. cc @kheradmandG for visibility. |
No description provided.