[P4Testgen] Unify compiler options and tool options. Ensure options context is always initialized correctly. #4787
+266
−334
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fruffy
force-pushed
the
fruffy/testgen_options_cleanup
branch
2 times, most recently
from
0bb6a4b
to
c478a18
Compare
fruffy
force-pushed
the
fruffy/testgen_options_cleanup
branch
2 times, most recently
from
cf25391
to
167fe11
Compare
fruffy
force-pushed
the
fruffy/testgen_options_cleanup
branch
from
167fe11
to
ba435db
Compare
fruffy
force-pushed
the
fruffy/testgen_options_cleanup
branch
2 times, most recently
from
269d0d3
to
093029b
Compare
Signed-off-by: fruffy <fruffy@nyu.edu>
fruffy
force-pushed
the
fruffy/testgen_options_cleanup
branch
from
093029b
to
b683f0d
Compare
fruffy
commented
Sep 13, 2024
|
|
||
| #include "backends/p4tools/common/lib/variables.h" | ||
| #include "ir/irutils.h" | ||
|
|
||
| namespace P4::P4Tools { | ||
|
|
||
| Target::Spec::Spec(std::string deviceName, std::string archName) | ||
| : deviceName(std::move(deviceName)), archName(std::move(archName)) { | ||
| Target::Spec::Spec(std::string_view deviceName, std::string_view archName) |
There was a problem hiding this comment.
Not sure about string_view here, can revert if necessary.
|
@vlstill Could you give this a review? This is a rather major change to the way options work for P4Testgen but is required to fix issues with using the tool as a library. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a larger rewrite of the core parts of P4Testgen's target initialization. The goal is to fix several problems when trying to use P4Testgen and other tools as a library. We need to make sure that the CompileContext we are accessing is the correct one.
We decouple initialization of the options and the initialization of the target. This way we can initialize a target before we even know which options we need. This means we can make the options target-dependent (e.g., different options for BMv2, Tofino, etc). This can be really nice for usability since we can create bespoke options per target.
To reduce the amount of random static objects floating around we fold P4ToolsOptions and the CompilerOptions together. There is no reason why need two classes here.
Options are only accessed by passing them along or retrieving them from the context stack. Ideally, we get rid of the context stack entirely but this requires a lot more work.