Add support for flattening header union stack #3890
Conversation
usha1830
changed the title
| @@ -106,9 +106,13 @@ set (P4_XFAIL_TESTS | |||
| testdata/p4_16_samples/pna-example-tcp-connection-tracking-err-1.p4 | |||
| testdata/p4_16_samples/pna-example-tcp-connection-tracking-err.p4 | |||
| testdata/p4_16_samples/pna-example-pass-2.p4 | |||
| testdata/p4_16_samples/invalid-hdr-warnings5.p4 | |||
There was a problem hiding this comment.
Maybe we should remove the check for conditional execution in actions for p4test.
There was a problem hiding this comment.
Yes. Many tests are failing for this.
Can we remove the check inside_action here?
https://github.com/p4lang/p4c/blob/main/midend/predication.h#L117
I see an earlier attempt from you to add a policy to predication pass
#3549
| @@ -153,6 +152,7 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) { | |||
| options.loopsUnrolling ? new P4::ParsersUnroll(true, &refMap, &typeMap) : nullptr, | |||
| evaluator, | |||
| [this, evaluator]() { toplevel = evaluator->getToplevelBlock(); }, | |||
| new P4::FlattenHeaderUnion(&refMap, &typeMap, options.loopsUnrolling), | |||
There was a problem hiding this comment.
why is loopsUnrolling used o control flattening?
There was a problem hiding this comment.
The flattening pass does not handle the stack accessors .next, .last, .lastIndex. Loops Unrolling unrolls the parser loops and concretizes the header (unions) stack indices. Hence, header union stack flattening should only be applied when the loops unrolling is enabled.
Do you have any suggestions on how these stack accessors (.next, .last etc) can be handled?
There was a problem hiding this comment.
I don't think there is an efficient way to do this is a target independent way. If you commit to a representation of next and valid you can do something nice. But probably not in the midend
| @@ -52,9 +52,3 @@ invalid-hdr-warnings7.p4(56): [--Wwarn=invalid_header] warning: accessing a fiel | |||
| invalid-hdr-warnings7.p4(57): [--Wwarn=invalid_header] warning: accessing a field of an invalid header us1[1].h1 | |||
| ^ us1[0].h2.a ^ us1[1].h1.a ^ us1[1].h2.a; | |||
| ^^^^^^^^^ | |||
| invalid-hdr-warnings7.p4(78): [--Wwarn=invalid_header] warning: accessing a field of an invalid header u.h1 | |||
There was a problem hiding this comment.
have you checked whether this error message should indeed disappear?
There was a problem hiding this comment.
From the comments in testcase, it looks like line :78 is not expected to be invalid and this change in output is OK.
Infact, as p4_16 spec section https://p4.org/p4-spec/docs/P4-16-working-spec.html#sec-expr-hu, I would think even line:79 is not invalid as assignment to header union element implicitly sets it to Valid.
78 u.h1.a = 1;
79 u.h2.a = 1; // expected invalid
However, these warnings are coming from frontend. The changes in this PR does not involve anything which should affect the output from frontend, so I am not sure what change in this PR is causing it to disappear. I will dig further up.
There was a problem hiding this comment.
I built the compiler from the main branch and ran this test with p4test. The warning for line:78 is not issued.
Perhaps the output in -stderr is not up-to-date and is not being checked as this is marked as XFAIL in p4test. Since, the FlattenHeaderUnion pass (which introduces conditional statements) is moved after Predication pass, the errors related to conditional statements in actions are not issued.
* Add support for header union stack * Minor changes * Address review comments * Fix lint errors
Flatten header union stack variables into its individual elements. All occurrences of the header union stack variables are replaced by the elements in the stack.
For ex:
is replaced by
References to u[0] is replaced by u0. Likewise for all stack elements.