Support for 128 bit bitwise operation #4952

Sosutha · 2024-10-09T04:42:40Z

Added movh statement definition
Added support for 128 bitwise operation
Updated testcase and output

psivanup

Please check the comments

psivanup · 2024-10-09T09:39:51Z

backends/dpdk/dpdkArch.h

+                }
+            }
+        }
+        return false;


Is it valid input to have operand1 of 128 bits and operand2 less than 128 bits at this stage?
If true, isValidOperandSize in line number 1106 may issue error for operand being greater than 64 bits.
IMO, isValidOperandSize function can be parameterized to check limit of 128 bits in exceptional cases.

Only operations where both operands are of size 128 bits are being supported for now.

Typechecking will ensure that the operands to any binary operation like these are the exact same type, so there's really no need to check both. The only binops that can have different types for their operands are shifts, array index, and concat. If you want to be paranoid, you can BUG_CHECK(src1Op->type == src2Op->type, ...

Updated the code.

backends/dpdk/dpdkHelpers.cpp

psivanup · 2024-10-09T09:43:01Z

backends/dpdk/dpdkHelpers.cpp

+    fields->push_back(new IR::StructField("tmp", IR::Type_Bits::get(64)));
+    const IR::Type_Header *headerStruct = new IR::Type_Header(IR::ID("tmp128_t"), *fields);
+    auto name = new cstring("tmp128_t");
+    structure->header_types.emplace(*name, headerStruct);


Suggested change

structure->header_types.emplace(*name, headerStruct);

structure->header_types.emplace(cstring("tmp128_t"), headerStruct);

Updated the code

KrisNey-MSFT · 2024-10-09T16:10:38Z

Suggest to try to compile DASH code as a test of completeness to see if the code compiles/builds? If DASH has other than bitwise operations, it may fail. Do not support additions to IPv6 addresses (we would likely not do this), but all else s/be supported. Can we add @jafingerhut as a reviewer please?

jafingerhut · 2024-10-10T15:36:01Z

Updated on 2024-Oct-10 to correct the p4c-dpdk command line options. I still get errors saying 'Unupported bitwidth 128' in them with the updated command line.

I tried checking out this modified branch of the code, and built p4c-dpdk, then tried the following commands:

git clone https://github.com/sonic-net/DASH
cd DASH/dash-pipeline/bmv2
mkdir -p output
p4c-dpdk -DTARGET_DPDK_PNA -DPNA_CONNTRACK --pp output/dash_pipeline.pp.p4 \
        -o output/dash_pipeline.spec --arch pna \
        --bf-rt-schema output/dash_pipeline.p4.bfrt.json \
        --p4runtime-files output/dash_pipeline.p4.p4info.txt \
        dash_pipeline.p4

I see many warnings, which are unrelated to these changes, but I also see 7 errors "Unsupported bitwidth 128" as copied below:

stages/../dash_routing_types.p4(130): [--Werror=target-error] error: Unsupported bitwidth 128 in underlay_sip
                            underlay_sip = underlay_sip == 0 ? hdr.u0_ipv4.src_addr : (IPv4Addres...
                                           ^^^^^^^^^^^^
stages/../dash_routing_types.p4(130): [--Werror=target-error] error: Unsupported bitwidth 128 in 128w0
...                underlay_sip = underlay_sip == 0 ? hdr.u0_ipv4.src_addr : (IPv4Address)underla...
                                                  ^
stages/../dash_routing_types.p4(131): [--Werror=target-error] error: Unsupported bitwidth 128 in underlay_dip
                            underlay_dip = underlay_dip == 0 ? hdr.u0_ipv4.dst_addr : (IPv4Addres...
                                           ^^^^^^^^^^^^
stages/../dash_routing_types.p4(131): [--Werror=target-error] error: Unsupported bitwidth 128 in 128w0
...                underlay_dip = underlay_dip == 0 ? hdr.u0_ipv4.dst_addr : (IPv4Address)underla...
                                                  ^
stages/../dash_routing_types.p4(203): [--Werror=target-error] error: Unsupported bitwidth 128 in overlay_sip_mask
...( (IPv6Address)hdr.u0_ipv4.src_addr & ~overlay_sip_mask) | overlay_sip) & ~meta.eni_data.pl_si...
                                          ^^^^^^^^^^^^^^^^
[--Werror=target-error] error: Unsupported bitwidth 128 in meta._eni_data_pl_sip_mask20
dash_pipeline.p4(229): [--Werror=target-error] error: Unsupported bitwidth 128 in meta._overlay_data_dip_mask112
        routing_action_apply.apply(hdr, meta);
                                        ^^^^
dash_pipeline.p4(229): [--Werror=target-error] error: Unsupported bitwidth 128 in meta._overlay_data_sip_mask111
        routing_action_apply.apply(hdr, meta);
                                        ^^^^

Do you see these as well, Sosutha?

It would be good to understand why these errors occur, and if possible, either modify this PR so that those errors do not occur, or figure out a modified version of the DASH program that behaves as it is written today, but does not give those errors.

fruffy

This PR should not be merged until the compilator for https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/dash/dash-pipeline-pna-dpdk.p4 passes

Sosutha · 2024-10-11T11:51:32Z

p4_16_samples/dash/dash-pipeline-pna-dpdk.p4

Andy, I am getting same errors as you. This PR addresses only 128 bitwise operations. I can add implementation for comparison operations to this same PR. So errors related to that will be solved with that implementation. For error in apply statement, I will have to check and revert back

fruffy · 2024-10-12T19:14:57Z

p4_16_samples/dash/dash-pipeline-pna-dpdk.p4

Andy, I am getting same errors as you. This PR addresses only 128 bitwise operations. I can add implementation for comparison operations to this same PR. So errors related to that will be solved with that implementation. For error in apply statement, I will have to check and revert back

We can also add the failing compilation to XFails for now. The important part is that this program is added as a test that is checked.

Sosutha · 2024-10-23T16:11:07Z

p4_16_samples/dash/dash-pipeline-pna-dpdk.p4

Andy, I am getting same errors as you. This PR addresses only 128 bitwise operations. I can add implementation for comparison operations to this same PR. So errors related to that will be solved with that implementation. For error in apply statement, I will have to check and revert back

We can also add the failing compilation to XFails for now. The important part is that this program is added as a test that is checked.

@fruffy @jafingerhut Now, support for bitwise, comparison(==, !=) and compliment operations needed for DASH Program are added in this same PR. Also failing compilation is added to XFails now. Now the errors are due to 128 bit constants in DASH P4 program which are not supported as of now. This PR can be merged with these operations and handling 128 bit constants can be taken as separate PR/issue.

Signed-off-by: Sosutha Sethuramapandian <[email protected]> * Added movh statement definition * Added support for 128 bitwise operation * Added support for 128 bit equal and not equal operators. * Added support for 128 bit compliment operation * Updated testcase and output

fruffy

Great, thank you adding the program as test. We can periodically update it.
The MacOS failure can be fixed with a rebase.

KrisNey-MSFT · 2024-10-23T17:19:56Z

[like] Kristina Moore reacted to your message:

…

________________________________ From: Fabian Ruffy ***@***.***> Sent: Wednesday, October 23, 2024 4:13:17 PM To: p4lang/p4c ***@***.***> Cc: Kristina Moore ***@***.***>; Comment ***@***.***> Subject: Re: [p4lang/p4c] Support for 128 bit bitwise operation (PR #4952) @fruffy approved this pull request. Great, thank you adding the program as test. We can periodically update it. The MacOS failure can be fixed with a rebase. — Reply to this email directly, view it on GitHub<#4952 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFJSI6FHK4ED4PEIV6NXUO3Z47DJ3AVCNFSM6AAAAABPTUPUXOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOBZGM2DCNBUGA>. You are receiving this because you commented.Message ID: ***@***.***>

psivanup

Looks fine to me

Sosutha mentioned this pull request Oct 9, 2024

P4 DPDK on horizon; BMV2 sonic-net/DASH#408

Open

Sosutha force-pushed the dpdk_128_bit_support branch from 9e9f473 to a0e34e4 Compare October 9, 2024 04:44

Sosutha mentioned this pull request Oct 9, 2024

p4c-dpdk back end does not support 128-bit arithmetic #4174

Open

psivanup reviewed Oct 9, 2024

View reviewed changes

Sosutha force-pushed the dpdk_128_bit_support branch 3 times, most recently from e20762a to 30b7c86 Compare October 9, 2024 10:45

fruffy added the dpdk Topics related to the DPDK back end label Oct 9, 2024

fruffy requested a review from jafingerhut October 9, 2024 16:30

Sosutha force-pushed the dpdk_128_bit_support branch 5 times, most recently from 015e0a9 to 80518e8 Compare October 10, 2024 12:50

fruffy requested changes Oct 10, 2024

View reviewed changes

Sosutha force-pushed the dpdk_128_bit_support branch 2 times, most recently from 0b866d7 to ae74cb8 Compare October 11, 2024 10:02

Sosutha force-pushed the dpdk_128_bit_support branch 9 times, most recently from 45bbda9 to d40ff2f Compare October 23, 2024 12:29

fruffy approved these changes Oct 23, 2024

View reviewed changes

Sosutha force-pushed the dpdk_128_bit_support branch from d40ff2f to ce5265d Compare October 23, 2024 16:13

Sosutha requested review from ChrisDodd and psivanup October 23, 2024 16:23

jafingerhut approved these changes Oct 23, 2024

View reviewed changes

Sosutha added this pull request to the merge queue Oct 24, 2024

Sosutha removed this pull request from the merge queue due to a manual request Oct 24, 2024

psivanup approved these changes Oct 24, 2024

View reviewed changes

Sosutha added this pull request to the merge queue Oct 24, 2024

Merged via the queue into p4lang:main with commit c9abd56 Oct 24, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for 128 bit bitwise operation #4952

Support for 128 bit bitwise operation #4952

Sosutha commented Oct 9, 2024

psivanup left a comment

psivanup Oct 9, 2024

Sosutha Oct 9, 2024

ChrisDodd Oct 9, 2024 •

edited

Loading

Sosutha Oct 10, 2024

psivanup Oct 9, 2024

Sosutha Oct 9, 2024

KrisNey-MSFT commented Oct 9, 2024 •

edited

Loading

jafingerhut commented Oct 10, 2024 •

edited

Loading

fruffy left a comment

Sosutha commented Oct 11, 2024

fruffy commented Oct 12, 2024

Sosutha commented Oct 23, 2024 •

edited

Loading

fruffy left a comment

KrisNey-MSFT commented Oct 23, 2024 via email

psivanup left a comment

	structure->header_types.emplace(*name, headerStruct);
	structure->header_types.emplace(cstring("tmp128_t"), headerStruct);

Support for 128 bit bitwise operation #4952

Support for 128 bit bitwise operation #4952

Conversation

Sosutha commented Oct 9, 2024

psivanup left a comment

Choose a reason for hiding this comment

psivanup Oct 9, 2024

Choose a reason for hiding this comment

Sosutha Oct 9, 2024

Choose a reason for hiding this comment

ChrisDodd Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Sosutha Oct 10, 2024

Choose a reason for hiding this comment

psivanup Oct 9, 2024

Choose a reason for hiding this comment

Sosutha Oct 9, 2024

Choose a reason for hiding this comment

KrisNey-MSFT commented Oct 9, 2024 • edited Loading

jafingerhut commented Oct 10, 2024 • edited Loading

fruffy left a comment

Choose a reason for hiding this comment

Sosutha commented Oct 11, 2024

fruffy commented Oct 12, 2024

Sosutha commented Oct 23, 2024 • edited Loading

fruffy left a comment

Choose a reason for hiding this comment

KrisNey-MSFT commented Oct 23, 2024 via email

psivanup left a comment

Choose a reason for hiding this comment

ChrisDodd Oct 9, 2024 •

edited

Loading

KrisNey-MSFT commented Oct 9, 2024 •

edited

Loading

jafingerhut commented Oct 10, 2024 •

edited

Loading

Sosutha commented Oct 23, 2024 •

edited

Loading