Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass to get rid of action_run() calls #5053

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ChrisDodd
Copy link
Contributor

This is a general midend pass for any target that does not support action_run directly. It rewrites the actions in a table that uses action_run to set a newly introduced metadata tmp enum that records which action was run, and changes the switch statement to use that.

It is kind-of logically the reverse of EliminateSwitch, which converts switch statements that don't use action_run() into ones that do, so it would never make sense to use both passes for a backend.

Need to update p4test so we can select which pass(es) to run in a test by some annotation in the test program to better allow testing this.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Dec 5, 2024
@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 6 times, most recently from bd15e5c to 7976cfe Compare December 10, 2024 01:00
@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 2 times, most recently from 89e94d6 to a26e2e0 Compare January 5, 2025 06:03
@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 2 times, most recently from 96ece1f to e448341 Compare February 3, 2025 04:35
@ChrisDodd ChrisDodd requested review from asl, vlstill and fruffy February 11, 2025 05:58
@ChrisDodd
Copy link
Contributor Author

I've added the ability to use @command_line annotations to add command-line args in test cases, so this case actually be tested

@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 2 times, most recently from f544827 to 2f64c87 Compare February 11, 2025 07:23
#include <core.p4>
#include <v1model.p4>

@command_line("--preferSwitch")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option will only be valid for the p4test back end but this is also a v1model program for BMv2.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4Testgen fails because it tries to execute this program but does not support this option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure such pragma is a good approach as this essentially adds a direct and unctrollable way to change internal compiler state. Even more, the option might be not supported at all, spelled differently, might not be used at all, etc. Why cannot some pragma with semantics be used instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the filename does not end in -bmv2.p4 it is not a valid bmv2 program (like many other tests in this directory) so should not be used by testgen (and in fact is not). So P4Testgen should not be looking at this file (and as far as I can tell, it does not -- all the testgen failures are with other source files).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4Testgen checks programs which include v1model.p4: https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/testgen/targets/bmv2/test/P4Tests.cmake#L9

Which, imho, is the right way to do because any program that includes this should run on BMv2 or others.

@fruffy
Copy link
Collaborator

fruffy commented Feb 11, 2025

The other failures is caused by this line

This is also not a supported option for P4Testgen but the pragma is still parsed as input.

@fruffy
Copy link
Collaborator

fruffy commented Feb 11, 2025

I see three possible ways to solve this:

  1. Since options may depend on the compiler executing the program, maybe extend this pragma with a "target"/"arch" field to ensure the option is interpreted correctly?

  2. Add another option to enable/disable this pragma and adding options.

  3. Or just ignore any unknown option made available by the pragma. Maybe print a warning that the option is not supported by the tool interpreting the program.

@ChrisDodd
Copy link
Contributor Author

The other failures is caused by this line

This is also not a supported option for P4Testgen but the pragma is still parsed as input.

This line was not touched by this change, so there's no change here

@fruffy
Copy link
Collaborator

fruffy commented Feb 12, 2025

This line was not touched by this change, so there's no change here

It's existing code that had no impact but is now active because of https://github.com/p4lang/p4c/pull/5053/files#diff-b033ed467d2767be363300d09ff13ac0a990d2e0de7464b732d9b29888da81a1R54.

- per-target selectable support, though most should want it

Signed-off-by: Chris Dodd <[email protected]>
- introduce new metadata set in the actions of the table

Signed-off-by: Chris Dodd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants