Skip to content

Pass to get rid of action_run() calls#5053

Merged
ChrisDodd merged 1 commit into
p4lang:mainfrom
ChrisDodd:cdodd-action-run
Mar 8, 2025
Merged

Pass to get rid of action_run() calls#5053
ChrisDodd merged 1 commit into
p4lang:mainfrom
ChrisDodd:cdodd-action-run

Conversation

@ChrisDodd

@ChrisDodd ChrisDodd commented Dec 5, 2024

Copy link
Copy Markdown
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, fruffy and vlstill February 11, 2025 05:58
@ChrisDodd

ChrisDodd commented Feb 11, 2025

Copy link
Copy Markdown
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
@fruffy fruffy mentioned this pull request Feb 11, 2025
Merged
fruffy
fruffy reviewed Feb 11, 2025
View reviewed changes
Comment thread testdata/p4_16_samples/elimActRun1.p4
@fruffy

fruffy commented Feb 11, 2025

Copy link
Copy Markdown
Collaborator

The other failures is caused by this line

p4c/backends/p4tools/modules/testgen/targets/tofino/test/p4-programs/tna_simple_switch.p4

Line 33 in c67d1fd

@command_line("--disable-parse-max-depth-limit")

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

@fruffy

fruffy commented Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

ChrisDodd commented Feb 11, 2025

Copy link
Copy Markdown
Contributor Author

The other failures is caused by this line

p4c/backends/p4tools/modules/testgen/targets/tofino/test/p4-programs/tna_simple_switch.p4

Line 33 in c67d1fd

@command_line("--disable-parse-max-depth-limit")

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

fruffy commented Feb 12, 2025

Copy link
Copy Markdown
Collaborator

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.

@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 2 times, most recently from f5d6769 to 5e02a50 Compare February 12, 2025 06:55
@ChrisDodd

ChrisDodd commented Feb 12, 2025

Copy link
Copy Markdown
Contributor Author

I modified P4COptionPragmaParser so that targets can decide whether or not they want to support @command_line option pragmas -- most real targets probably want to, but special targets like p4testgen and p4graphs possibly do not. I also remove <v1model.p4> from the testcase so p4testgen doesn't pick it up.

@ChrisDodd ChrisDodd requested a review from fruffy February 12, 2025 21:46
@fruffy fruffy added the run-validation Use this tag to trigger a Validation CI run. label Feb 12, 2025
@fruffy

fruffy commented Feb 12, 2025

Copy link
Copy Markdown
Collaborator

Will be able to review the action_run pass later this week. But I'd suggest to split out the changes to P4COptionPragmaParser into a PR we can discuss separately. I think it is a cool and useful feature but it shouldn't be tagged on to this PR.

@ChrisDodd

ChrisDodd commented Feb 13, 2025

Copy link
Copy Markdown
Contributor Author

Will be able to review the action_run pass later this week. But I'd suggest to split out the changes to P4COptionPragmaParser into a PR we can discuss separately. I think it is a cool and useful feature but it shouldn't be tagged on to this PR.

Created #5123

fruffy
fruffy reviewed Feb 16, 2025
View reviewed changes
Comment thread midend/eliminateActionRun.h
Comment thread midend/eliminateActionRun.h Outdated
Comment thread midend/eliminateActionRun.cpp Outdated
@fruffy

fruffy commented Feb 18, 2025

Copy link
Copy Markdown
Collaborator

Could you remove the code that is related to #5123. Or do you want to rebase it on top of that PR?

Would be good to get a review from other stake holders here @kfcripps @vlstill.

@ChrisDodd

ChrisDodd commented Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Could you remove the code that is related to #5123. Or do you want to rebase it on top of that PR?

It requires that PR (for the testcase included here), and is (re)based on that. Ideally, that one will be approved (and merge first), then this one will rebase and become just the one change.

@fruffy

fruffy commented Feb 18, 2025

Copy link
Copy Markdown
Collaborator

It requires that PR (for the testcase included here), and is (re)based on that. Ideally, that one will be approved (and merge first), then this one will rebase and become just the one change.

It might take a little longer until #5123 is approved. @asl also had apprehensions.

This PR could be merged before that, given the tests pass and look ok.

kfcripps
kfcripps reviewed Feb 19, 2025
View reviewed changes
Comment thread midend/eliminateActionRun.h Outdated
Comment thread midend/eliminateActionRun.h Outdated
Comment thread backends/p4test/p4test.cpp Outdated
Comment thread midend/eliminateActionRun.cpp Outdated
Comment thread midend/eliminateActionRun.cpp Outdated
@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 4 times, most recently from 841a8fb to ececb06 Compare February 26, 2025 03:49
@ChrisDodd ChrisDodd requested review from fruffy and kfcripps March 4, 2025 21:38
@ChrisDodd ChrisDodd force-pushed the cdodd-action-run branch 2 times, most recently from ca82e11 to 977125f Compare March 5, 2025 10:42
@ChrisDodd
Pass to get rid of action_run() calls
977125f 
- introduce new metadata set in the actions of the table

Signed-off-by: Chris Dodd <cdodd@nvidia.com>
@ChrisDodd ChrisDodd added this pull request to the merge queue Mar 8, 2025
Merged via the queue into p4lang:main with commit 6ad7c9a Mar 8, 2025
@ChrisDodd ChrisDodd deleted the cdodd-action-run branch March 8, 2025 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asl asl asl approved these changes

@kfcripps kfcripps kfcripps approved these changes

@vlstill vlstill Awaiting requested review from vlstill

@fruffy fruffy Awaiting requested review from fruffy

Assignees

No one assigned

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser) run-validation Use this tag to trigger a Validation CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ChrisDodd @fruffy @asl @kfcripps