Make P4-14 optional. - #5399
Conversation
fruffy
added
bmv2
a85b447 to
8f41873
Compare
ChrisDodd
left a comment
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. |
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. |
a081d51 to
ad5f087
Compare
189b7b2 to
ae41d34
Compare
dc47ee3 to
6ddcd13
Compare
asl
left a comment
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?
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 |
da08e66 to
bfa7a48
Compare
fruffy
added
run-sanitizer
d85a815 to
7213a10
Compare
| #ifdef SUPPORT_P4_14 | ||
| CompilerOptions::FrontendVersion version) { | ||
| #else | ||
| CompilerOptions::FrontendVersion /*version*/) { |
There was a problem hiding this comment.
maybe this argument should go away completely if !SUPPORT_P4_14?
| ID Name() const { return obj->to<IInstance>()->Name(); } | ||
| dbprint { out << obj->to<IInstance>()->Name(); } | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems to be a noop change? This codeblock has just been moved up in the file with no changes.
| @@ -0,0 +1,55 @@ | |||
| /* | |||
There was a problem hiding this comment.
probably makes sense to move this file into frontends/p4-14
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
…lated to the bmv2 v1model. Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
Signed-off-by: fruffy <fruffy@nyu.edu>
4 participants
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.