[#4614] Unconditionally copy `dpdk` p4include files to the binary directory #4615

kfcripps · 2024-04-12T17:04:00Z

~~Moves all psa and pna dpdk tests to p4_16_pna_samples/ and p4_16_psa_samples/ so that the check-p4 testing target does not run them.~~
Unconditionally copy dpdk p4include files to the binary directory so that they will be available for the check-p4 testing target even when the dpdk backend is disabled.

Fixes #4614

This reverts commit 54ee318.

fruffy · 2024-04-14T22:18:02Z

CMakeLists.txt

@@ -539,18 +539,13 @@ add_custom_target(update_includes ALL
  COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/*.p4 ${P4C_BINARY_DIR}/p4include
  COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/bmv2
  COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/bmv2/psa.p4 ${P4C_BINARY_DIR}/p4include/bmv2
+  COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/dpdk


Add a TODO pointing to #4614 that we should clean this up?

By "clean this up," do you mean changing the tests' <pna.p4> includes to either <bmv2/pna.p4> or <dpdk/pna.p4>? Or do you mean that check-p4 should not run these dpdk or bmv2 tests at all?

I think both cases will end up being the result of resolving #4614. We can also put the TODO as deciding whether to fully disable these tests.

Is there any downside to running them under check-p4? My understanding now is that for all of the bmv2 and dpdk tests, check-p4 is just checking the frontend-generated and midend-generated IR and/or diagnostics, so they would still be useful to run. However if we are checking what is generated by e.g. the dpdk pna backend, then it wouldn't make sense to run the dpdk pna tests under check-p4

But it definitely makes sense to at least make the includes more verbose. I can try doing that in this PR later.

kfcripps · 2024-04-15T20:15:06Z

But it definitely makes sense to at least make the includes more verbose. I can try doing that in this PR later.

@fruffy Done.

For any #include <psa.p4> with "bmv2" in the test's name -> #include <bmv2/psa.p4>
For any #include <psa.p4> with "dpdk" in the test's name -> #include <dpdk/psa.p4>
For any #include <psa.p4> with neither "dpdk" nor "bmv2" in the test's name -> checked the history to see if the PR that added it was dpdk or bmv2 related:
- If dpdk-related -> #include <dpdk/psa.p4>
- If bmv2-related -> #include <bmv2/psa.p4>
- If related to neither -> default to #include <bmv2/psa.p4>
Did same as the above for any #include "psa.p4"
For any #include <pna.p4> or #include "pna.p4" -> #include <dpdk/pna.p4> (there is no bmv2/pna.p4)

…cific externs

kfcripps · 2024-04-15T21:00:57Z

Hmm, I just noticed that we also have p4include/pna.p4 in addition to the dpdk-specific p4include/dpdk/pna.p4. As far as I understand, it doesn't make sense to include the dpdk-specific pna.p4 unless the test requires one of the dpdk-specific extern functions not present in the generic pna.p4 file, so I have also replaced all #include <dpdk/pna.p4> with #include <pna.p4> wherever dpdk/pna.p4 is not needed.

fruffy

Thanks a lot! This a great cleanup. @jafingerhut could you also take a look?

fruffy · 2024-04-16T15:11:09Z

testdata/p4_16_samples_outputs/psa-example-select_tuple-wc.p4

@@ -1,5 +1,5 @@
 #include <core.p4>
-#include <bmv2/psa.p4>


Are these changes from bmv2/psa to dpdk/psa because the file is DPDK-related?

testdata/p4_16_samples/psa-example-select_tuple-wc.p4 was added in a dpdk-related PR (#2836), so the intent was likely to include dpdk/psa.p4. However, testdata/p4_16_samples/psa-example-select_tuple-wc.p4 contained #include <psa.p4>, so bmv2/psa.p4 was actually included.

jafingerhut · 2024-04-16T17:58:19Z

FYI, in case this isn't obvious to everyone:

It is expected that different target devices that implement PSA, or that implement PNA, architectures can customize the bit widths of several types, e.g. PortId_t, for that particular target device. Most P4 programs written for such an architecture will likely compile and run tests in this repo fine regardless of the particular value for those bit widths.

Some implementations have chosen to make additional customizations to pna.p4, e.g. dpdk/pna.p4 has additional changes to some parameters of some extern methods. As @kfcripps pointed out earlier, many particular test programs might not be using such methods, and those will likely work identically regardless of which version of the pna.p4 include file they use.

While we could consider trying to divide up PNA architecture test programs into these 3 categories:

(a) ones that must use a BMv2-specific pna.p4 include file (assuming one exists in the near future, as appears likely given other PRs in the works)
(b) ones that must use a DPDK-specific pna.p4 include file
(c) ones that can use "a more generic pna.p4 include file"

Note that if in the future the DPDK-specific pna.p4 include file were to change (as an example, but this is not specific to DPDK), it might require moving test programs around that were formerly in group (c) into group (b).

I am not personally strongly against such an approach, but would like to suggest that it might be less busy-work for everyone involved if we DO NOT attempt to have category (c) tests at all. That is, every test program for the PNA architecture would be focused solely on testing one particular target.

If we take that approach, yes, then each target-implementer of an architecture gets complete control over what their test programs are, and all of them would include their target-specific version of the pna.p4 include file, and it would be up to that team of people to decide exactly what their test programs test, vs. what they do not.

That approach also makes it easier to manage test programs in the currently-common situation that different targets implement different subsets of an architecture specification.

kfcripps · 2024-04-16T19:57:29Z

Note that if in the future the DPDK-specific pna.p4 include file were to change (as an example, but this is not specific to DPDK), it might require moving test programs around that were formerly in group (c) into group (b).

If a test is already in group (c) then why would changing the dpdk-specific pna.p4 file require moving the test to group (b)?

I am not personally strongly against such an approach, but would like to suggest that it might be less busy-work for everyone involved if we DO NOT attempt to have category (c) tests at all. That is, every test program for the PNA architecture would be focused solely on testing one particular target.

If we take that approach, yes, then each target-implementer of an architecture gets complete control over what their test programs are, and all of them would include their target-specific version of the pna.p4 include file, and it would be up to that team of people to decide exactly what their test programs test, vs. what they do not.

That approach also makes it easier to manage test programs in the currently-common situation that different targets implement different subsets of an architecture specification.

I don't really have a preference between always including the target-specific pna.p4 vs. sometimes including the generic pna.p4 file so I'll let you two decide @fruffy @jafingerhut and then I can update the includes touched by this PR accordingly.

jafingerhut · 2024-04-16T21:36:37Z

@kfcripps asked: "If a test is already in group (c) then why would changing the dpdk-specific pna.p4 file require moving the test to group (b)?"

Because the P4 program being tested uses some feature in the dpdk-specific pna.p4 file that the DPDK developers decided to modify, similarly to how at some point in the past they decided to modify the signatures of some extern methods to take additional parameters. They might decide to do so again in the future (or another target might), and if any test P4 programs used that method, it would need to be updated to continue compiling and running for the target.

kfcripps · 2024-04-16T21:47:27Z

@kfcripps asked: "If a test is already in group (c) then why would changing the dpdk-specific pna.p4 file require moving the test to group (b)?"

Because the P4 program being tested uses some feature in the dpdk-specific pna.p4 file that the DPDK developers decided to modify, similarly to how at some point in the past they decided to modify the signatures of some extern methods to take additional parameters. They might decide to do so again in the future (or another target might), and if any test P4 programs used that method, it would need to be updated to continue compiling and running for the target.

@jafingerhut But if a test is in group (c), that means it's not including the dpdk-specific pna.p4 file, and therefore not using some dpdk-specific feature, right?

kfcripps · 2024-04-16T21:54:46Z

@jafingerhut Do you mean that a test might need to be moved from group (c) to (b) if:

DPDK makes a dpdk-specific change to some existing feature that is currently exposed in the generic pna.p4 file, and
The test uses this feature in some way, and
The test belongs to the set of tests added to in P4_16_SUITES in https://github.com/p4lang/p4c/blob/main/backends/dpdk/CMakeLists.txt, and
We want the test to continue to exist among P4_16_SUITES for some reason

?

kfcripps · 2024-04-18T13:32:31Z

@fruffy @jafingerhut Should I merge this as is? Or shall I remove all #include <pna.p4> first?

fruffy · 2024-04-18T14:15:32Z

I lost track of the discussion.

I do like the shared approach solely for the reason that we are looking into developing a BMv2 PNA target #4606 and can reuse all the programs with generic includes as test programs.

The alternative to this approach, without duplicating test programs, is to use #define directives which include the right header depending on the compiler. But that requires even more rewriting work.

kfcripps · 2024-04-18T16:33:50Z

I do like the shared approach solely for the reason that we are looking into developing a BMv2 PNA target #4606 and can reuse all the programs with generic includes as test programs.

Ok, I will merge this as-is if the changes look ok to you @jafingerhut?

jafingerhut · 2024-04-18T17:24:34Z

@kfcripps I will approve this PR in a moment. Please go ahead and we can work out later if there are more changes we want to make. Thanks.

jafingerhut

LGTM

kfcripps · 2024-04-18T17:28:31Z

@jafingerhut @fruffy Thanks!

Omit psa and pna dpdk tests under check-p4 testing target

54ee318

kfcripps requested review from asl and fruffy April 12, 2024 17:04

kfcripps mentioned this pull request Apr 12, 2024

PNA and PSA DPDK tests should not be run by check-p4 target #4614

Closed

kfcripps added 2 commits April 12, 2024 15:45

Revert "Omit psa and pna dpdk tests under check-p4 testing target"

8dc1140

This reverts commit 54ee318.

Copy dpdk p4include files even when dpdk backend is not enabled.

a2b9d61

kfcripps changed the title ~~[#4614] Omit psa and pna dpdk tests under check-p4 testing target~~ [#4614] Unconditionally copy dpdk p4include files to the binary directory Apr 12, 2024

kfcripps requested a review from jafingerhut April 13, 2024 00:01

fruffy reviewed Apr 14, 2024

View reviewed changes

fruffy approved these changes Apr 14, 2024

View reviewed changes

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Apr 14, 2024

kfcripps added 6 commits April 15, 2024 11:54

Disambiguate all psa.p4 includes

a71cbac

Update test outputs

2108e9b

Disambiguate all pna.p4 includes

3734a7a

Update test outputs

7cece49

Use angled brackets instead of double quotes for psa and pna includes

eb03ebd

Update test outputs

a301402

kfcripps requested a review from fruffy April 15, 2024 20:15

kfcripps added 3 commits April 15, 2024 13:19

Add TODO comment referencing issue p4lang#4614

b2f288a

Only #include <dpdk/pna.p4> for tests which actually require dpdk-spe…

6a71a25

…cific externs

Update test outputs

512f30f

fruffy reviewed Apr 16, 2024

View reviewed changes

fix #include typo

5fb8bcf

jafingerhut approved these changes Apr 18, 2024

View reviewed changes

kfcripps added this pull request to the merge queue Apr 18, 2024

Merged via the queue into p4lang:main with commit 81af965 Apr 18, 2024
17 checks passed

kfcripps deleted the 4614 branch April 18, 2024 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4614] Unconditionally copy `dpdk` p4include files to the binary directory #4615

[#4614] Unconditionally copy `dpdk` p4include files to the binary directory #4615

kfcripps commented Apr 12, 2024 •

edited

Loading

fruffy Apr 14, 2024

kfcripps Apr 14, 2024

fruffy Apr 14, 2024

kfcripps Apr 14, 2024

kfcripps Apr 14, 2024

kfcripps commented Apr 15, 2024 •

edited

Loading

kfcripps commented Apr 15, 2024

fruffy left a comment

fruffy Apr 16, 2024 •

edited

Loading

kfcripps Apr 16, 2024

jafingerhut commented Apr 16, 2024

kfcripps commented Apr 16, 2024

jafingerhut commented Apr 16, 2024

kfcripps commented Apr 16, 2024

kfcripps commented Apr 16, 2024

kfcripps commented Apr 18, 2024

fruffy commented Apr 18, 2024

kfcripps commented Apr 18, 2024

jafingerhut commented Apr 18, 2024

jafingerhut left a comment

kfcripps commented Apr 18, 2024

[#4614] Unconditionally copy dpdk p4include files to the binary directory #4615

[#4614] Unconditionally copy dpdk p4include files to the binary directory #4615

Conversation

kfcripps commented Apr 12, 2024 • edited Loading

fruffy Apr 14, 2024

Choose a reason for hiding this comment

kfcripps Apr 14, 2024

Choose a reason for hiding this comment

fruffy Apr 14, 2024

Choose a reason for hiding this comment

kfcripps Apr 14, 2024

Choose a reason for hiding this comment

kfcripps Apr 14, 2024

Choose a reason for hiding this comment

kfcripps commented Apr 15, 2024 • edited Loading

kfcripps commented Apr 15, 2024

fruffy left a comment

Choose a reason for hiding this comment

fruffy Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

kfcripps Apr 16, 2024

Choose a reason for hiding this comment

jafingerhut commented Apr 16, 2024

kfcripps commented Apr 16, 2024

jafingerhut commented Apr 16, 2024

kfcripps commented Apr 16, 2024

kfcripps commented Apr 16, 2024

kfcripps commented Apr 18, 2024

fruffy commented Apr 18, 2024

kfcripps commented Apr 18, 2024

jafingerhut commented Apr 18, 2024

jafingerhut left a comment

Choose a reason for hiding this comment

kfcripps commented Apr 18, 2024

[#4614] Unconditionally copy `dpdk` p4include files to the binary directory #4615

[#4614] Unconditionally copy `dpdk` p4include files to the binary directory #4615

kfcripps commented Apr 12, 2024 •

edited

Loading

kfcripps commented Apr 15, 2024 •

edited

Loading

fruffy Apr 16, 2024 •

edited

Loading