Make P4-14 optional. #5399
Make P4-14 optional. #5399
Conversation
Signed-off-by: fruffy <[email protected]>
ChrisDodd
left a comment
There was a problem hiding this comment.
We'll need to ensure some CI builds are built with P4_14 and some without. Not sure we need every possible combination of OS + build tool + P4_14 on/off + unity on/off, but at least some orthogonal coverage.
| } | ||
|
|
||
| /// FIXME: Only required for P4-14. Remove. | ||
| class Attribute : Declaration { |
There was a problem hiding this comment.
why move this here from ir-v1.def? That seems backwards as you'll get it built in to the compiler even when P4_14 is disabled?
There was a problem hiding this comment.
Attribute is part of Type_Extern. And the ir-generator does not have support for preprocessor directives to conditionally ignore segments. If I do not include it optional inline NameMap<Attribute, ordered_map> attributes; // P4_14 only, currently a few lines below will produce an error.
Yes, my plan is to have one CI run that still builds with P4-14. I opened this PR a bit prematurely, there are still some changes necessary. P4-14 constructs are spread all across the front and mid end. |
Signed-off-by: fruffy <[email protected]>
007402c to
c82e144
Compare
|
@smolkaj This PR makes the P4-14 parts optional. With respect to Bazel, I can remove the P4-14 parts from the build system (e.g. v1lexer) or enable building with P4-14 (this is the previous behavior). What do you prefer? I can also try to add a toggle like we do in CMake, but I am not sure whether Bazel supports that well. |
Signed-off-by: fruffy <[email protected]>
…lated to the bmv2 v1model. Signed-off-by: fruffy <[email protected]>
fd27e65 to
189b7b2
Compare
Signed-off-by: fruffy <[email protected]>
b4923e1 to
dc47ee3
Compare
Signed-off-by: fruffy <[email protected]>
asl
left a comment
There was a problem hiding this comment.
See some comments. I'm not a huge fan of bunch of ifdefs spread across the whole frontend... Maybe this could be composed in some other way?
|
|
||
| } // namespace P4 | ||
|
|
||
| #ifdef SUPPORT_P4_14 |
There was a problem hiding this comment.
Maybe it's worth to split this into a separate header rather than throwing ifdef?
| return expression; | ||
| } | ||
|
|
||
| #ifdef SUPPORT_P4_14 |
|
|
||
| } // namespace P4 | ||
|
|
||
| #ifdef SUPPORT_P4_14 |
There was a problem hiding this comment.
Could it be split into a separate file?
|
|
||
| #ifdef SUPPORT_P4_14 | ||
| TEST_F(P4CParserUnroll, switch_20160512) { | ||
| auto parsers = loadExample("../p4_14_samples/switch_20160512/switch.p4", |
There was a problem hiding this comment.
This is a very useful test for the compiler performance. Maybe re-instate its converted version?
I don't think we need P4-14 support. I just want to make sure i understand -- P4-16 programs using v1model will continued to work? |
I am converting some of this into issues, e.g., #5402. I also want to reduce the #ifdefs as much as possible but some refactorings are a little bit more involved (for example, splitting out the v1parser). |
Yes, this will not affect P4-16 programs, but you might lose the ability to compile P4-14 programs. I do not think you work with those. 6ddcd13 is a possible implementation in Bazel |
Fixes #4776. Add a CMake option to enable P4-14. Otherwise, add a bunch of code pragmas to selectively disable P4-14 constructs. We can clean these up over time.
The move of v1model.h to the BMv2 back end is intentional. With that it becomes clear which front/mid end segments are "dirty", i.e., they depend on back-end-specific code.