Added support for 128 bit constants usage in dpdk backend #5074
Conversation
Updated testcases and outputs This PR overwrites the PR p4lang#5009 Signed-off-by: Maheswari Subramanian <[email protected]>
|
@jafingerhut, @fruffy, @psivanup, I created this PR for DPDK 128Bit constant support as I could not get rid of DCO check fail in another PR #5034. Now, in this PR everything is clean. Also we did execution test with this change and it works fine. Please review. |
backends/dpdk/DpdkXfail.cmake
Outdated
| @@ -26,7 +26,7 @@ p4c_add_xfail_reason("dpdk" | |||
| testdata/p4_16_samples/pna-dpdk-invalid-hdr-warnings5.p4 | |||
| testdata/p4_16_samples/pna-dpdk-invalid-hdr-warnings6.p4 | |||
| testdata/p4_16_samples/pna-dpdk-header-union-stack2.p4 | |||
| testdata/p4_16_samples/dash/dash-pipeline-pna-dpdk.p4 | |||
| testdata/p4_16_dpdk_errors/psa-dpdk-128bitCast_error.p4 | |||
There was a problem hiding this comment.
I thought that for tests in the p4_16_dpdk_errors (and other testdata/*_errors directories), a passing test is when the compiler produces the expected error messages in the output, which you have as part of this PR for psa-dpdk-128bitCast_error.p4, so that is good. But why is it in the list of tests marked as expected to fail? Do DPDK tests use Xfail differently than other p4c test programs?
|
These expected output files should be removed, as the test program they are for was moved from the errors directory to the samples directory:
|
from XFail list. Signed-off-by: Maheswari Subramanian <[email protected]>
|
@jafingerhut, I addressed your comments. Please check now. |
There was a problem hiding this comment.
I have reviewed the new test programs and their expected output files, and they look good to me. It would be nice if P4 DPDK supported arbitrary constant sizes from 1 up to 128 bits and all sizes in between, but supporting 128, and all sizes in the range 1 through 64 bits, is better than not supporting 128-bit values, certainly. I have not attempted to review the C++ parts of the changes, as that is outside of my expertise.
|
@jafingerhut , Thanks for approving. I don't have write access. Could you try to merge this PR? |
|
Given the list of approvers on #5009, where the C++ code changes are (I believe) the same, merging this. |
Updated testcases and outputs
This PR overwrites the PRs #5009 and #5034