Support for 128 bit constants in dpdk backend #5009

Sosutha · 2024-11-11T05:07:50Z

Signed-off-by : Sosutha Sethuramapandian [email protected]

Added support for 128 bit constants usage in dpdk backend
Updated testcases and outputs

fruffy · 2024-11-11T23:41:05Z

Great! @jafingerhut Do we have manual STF or PTF tests for the DASH program that could check for correctness here? We tried to automate this process once but removed the automated DPDK PTF tests because of stability issues.

jafingerhut · 2024-11-12T04:32:16Z

Great! @jafingerhut Do we have manual STF or PTF tests for the DASH program that could check for correctness here? We tried to automate this process once but removed the automated DPDK PTF tests because of stability issues.

There is an IPDK environment that you can run the DPDK software switch inside of, which adds a P4Runtime API server, but as far as I know the open source version of that has not been updated since July 2023. I could be wrong about that.

Without using that P4Runtime API server, my belief is that the there is only a native P4 DPDK control plane API that one would need to use in order to add table entries. I have not used that API before, nor even where to find documentation for it, but I haven't looked hard.

syogender · 2024-11-12T07:04:45Z

backends/dpdk/dpdkHelpers.cpp

@@ -632,7 +632,11 @@ bool ConvertStatementToDpdk::preorder(const IR::AssignmentStatement *a) {
        }
    } else if (right->is<IR::PathExpression>() || right->is<IR::Member>() ||
               right->is<IR::BoolLiteral>() || right->is<IR::Constant>()) {
-        i = new IR::DpdkMovStatement(a->left, a->right);
+        if (right->is<IR::Constant>() && right->type->to<IR::Type_Bits>()->width_bits() == 128) {


What happen for value greater than 64 bits and less than 128 bits

DPDK Backend Instructions support only 128 bit constants. If the constant is defined with 128 as width but value is less than 128 bits, it is still supported. Example: 128w0x1234ABCDEF12345678.
If the width is specified less than 128 but more than 64, then support is not available

Are you throwing an error for these cases? If so, where?

It will be thrown in dpdkArch.h itself, These cases will not reach this point of code.

syogender · 2024-11-12T07:09:40Z

backends/dpdk/dpdkArch.cpp

+        auto rightType = assn->right->type->to<IR::Type_Bits>()->width_bits();
+        if (leftType == 128 && rightType == leftType) {
+            auto cst = assn->right->to<IR::Constant>();
+            if (!cst->fitsUint64()) return assn;


how constant not fitting in uint64 will be handled?

[backends/dpdk/dpdkHelpers.cpp] - This file has the changes in this same PR where Unsigned values more than 64 bit is handled.

Why can the 128 bit case not be handled here? I am a little concerned that we might be masking failures here.

Also: the error message at line 2977 is incorrect and probably similar error messages, too. It looks like the DPDK backend only supports bit widths with power of 2, is that correct?

Code generation for 128 bit constant is handled in dpdkHelpers.cpp since here only the constraints needed for the DPDK is checked in this file. DPDK backend supports till 64 bits and assembly instructions to handle only 128 bit after 64 bits. Hence for the intermediate number of bits, error will be thrown.

To make sure this is working as intended I would example programs that trigger these error cases. Then we can also see what kind of error is thrown and whether that error makes sense.

syogender

lgtm

psivanup

Looks fine to me

Sosutha · 2024-11-13T06:05:28Z

@fruffy @jafingerhut Could you please approve this PR for merging

KrisNey-MSFT · 2024-11-13T17:12:18Z

Will we try executing the spec file to check whether this is working? bmv2 and dpdk targets/backends. @chrispsommers for visibility

Are DASH blockers fixed, and
can we run P4 DPDK and execute the DASH code in the repo?

We can pick this up in the Behavioral Model call tomorrow Nov 14 2024

fruffy · 2024-11-13T17:20:27Z

backends/dpdk/dpdkArch.cpp

+    if (isLargeFieldOperand(assn->left) && assn->right->is<IR::Constant>()) {
+        auto leftType = assn->left->type->to<IR::Type_Bits>()->width_bits();
+        auto rightType = assn->right->type->to<IR::Type_Bits>()->width_bits();
+        if (leftType == 128 && rightType == leftType) {


128 should be static constant that you define, maybe in dpdkHelpers.h. It should be used everywhere you use "128".

I would consider wrapping this in a namespace or struct to avoid conflicts. You can then access the value like DpdkConstants::SupportedInstructionWidth or something similar. Capturing what this restriction is meant to represent.

Signed-off-by : Sosutha Sethuramapandian <[email protected]> * Added support for 128 bit constants usage in dpdk backend * Updated testcases and outputs

maheswari-s

Looks fine

fruffy

Approving to unblock things but would be good if @jafingerhut could give this another pass before merging. It's hard for me to tell whether this is doing the right thing or implements the intended outcome.

jafingerhut · 2024-11-21T04:05:10Z

So I tried the attached P4 program, which requires you to use -DBITWIDTH=<number> on the command line to compile it, with all bit widths from 1 to 128, inclusive.

For bit widths 1 through 64, 72, 80, 88, 96, 104, 112, 120, and 128, it gave no error, and seemed to produce a reasonable .spec file, at least as long as assigning an action parameter value with width > 64 bits but < 128 bits to a 128-bit field is supposed to be legal in a spec file.

However, for other bit widths, i.e. those greater than 64 bits but less than 128 bits that were not a multiple of 8, it gave a CompilerBug error like the example below:

$ $HOME/forks2/p4c/build/p4c-dpdk -DBITWIDTH=65 --target dpdk --arch pna -o assign-bitX-to-bit128.spec assign-bitX-to-bit128.p4
terminate called after throwing an instance of 'P4::Util::CompilerBug'
  what():  In file: /home/andy/forks2/p4c/backends/dpdk/dpdkUtils.cpp:157
Compiler Bug: Metadata bit-width expected to be within 64-bits, found 65

Aborted (core dumped)

First, it should not core dump or get to a Compiler Bug.

Second, if you want it to give an error message, the message seems misleading, because it seems to be supporting widths larger than 64 bits, just not all bit widths in the range 65 to 128.

Third, if it actually works for bit widths 72, 80, 88, 96, 104, 112, 120, why not all of the others in the range 65 to 127?
assign-bitX-to-bit128.p4.txt

Also, it gives no error for bit widths that are a multiple of 8 larger than 128, e.g. 136, 144, etc. Are those supported, or should they give an error message?

Sosutha force-pushed the dpdk_128_bit branch 2 times, most recently from 6ca3177 to 49488f6 Compare November 11, 2024 07:50

fruffy added the dpdk Topics related to the DPDK back end label Nov 11, 2024

syogender reviewed Nov 12, 2024

View reviewed changes

syogender approved these changes Nov 12, 2024

View reviewed changes

psivanup approved these changes Nov 12, 2024

View reviewed changes

Sosutha force-pushed the dpdk_128_bit branch from 49488f6 to e75293f Compare November 13, 2024 08:48

fruffy reviewed Nov 13, 2024

View reviewed changes

Sosutha force-pushed the dpdk_128_bit branch 2 times, most recently from 9fb6064 to e28be7c Compare November 15, 2024 07:04

Sosutha requested a review from fruffy November 18, 2024 06:38

Sosutha force-pushed the dpdk_128_bit branch 2 times, most recently from 5ffc0a9 to c9f778b Compare November 19, 2024 07:21

Support for 128 bit constants in dpdk backend

b06a368

Signed-off-by : Sosutha Sethuramapandian <[email protected]> * Added support for 128 bit constants usage in dpdk backend * Updated testcases and outputs

Sosutha force-pushed the dpdk_128_bit branch from c9f778b to b06a368 Compare November 19, 2024 07:32

maheswari-s approved these changes Nov 20, 2024

View reviewed changes

fruffy approved these changes Nov 21, 2024

View reviewed changes

maheswari-s mentioned this pull request Nov 26, 2024

Support for 128 bit constants in dpdk backend #5034

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for 128 bit constants in dpdk backend #5009

Support for 128 bit constants in dpdk backend #5009

Sosutha commented Nov 11, 2024

fruffy commented Nov 11, 2024

jafingerhut commented Nov 12, 2024

syogender Nov 12, 2024

Sosutha Nov 12, 2024

fruffy Nov 13, 2024

Sosutha Nov 15, 2024

syogender Nov 12, 2024

Sosutha Nov 12, 2024

fruffy Nov 13, 2024

Sosutha Nov 15, 2024 •

edited

Loading

fruffy Nov 18, 2024

syogender left a comment

psivanup left a comment

Sosutha commented Nov 13, 2024

KrisNey-MSFT commented Nov 13, 2024

fruffy Nov 13, 2024

Sosutha Nov 18, 2024

fruffy Nov 18, 2024

maheswari-s left a comment

fruffy left a comment

jafingerhut commented Nov 21, 2024 •

edited

Loading

Support for 128 bit constants in dpdk backend #5009

Are you sure you want to change the base?

Support for 128 bit constants in dpdk backend #5009

Conversation

Sosutha commented Nov 11, 2024

fruffy commented Nov 11, 2024

jafingerhut commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sosutha Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syogender left a comment

Choose a reason for hiding this comment

psivanup left a comment

Choose a reason for hiding this comment

Sosutha commented Nov 13, 2024

KrisNey-MSFT commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maheswari-s left a comment

Choose a reason for hiding this comment

fruffy left a comment

Choose a reason for hiding this comment

jafingerhut commented Nov 21, 2024 • edited Loading

Sosutha Nov 15, 2024 •

edited

Loading

jafingerhut commented Nov 21, 2024 •

edited

Loading