Align key components to the nearest 8-bit size #30

komaljai · 2024-10-03T13:12:18Z

No description provided.

vbnogueira

I found some small issues, but the general idea of storePrimitive* and getPrimitive* makes sense

However, I have a question.
In cases where you have something like a bit<22>.
Is getPrimitive32's first parameter guaranteed to be zeroed out?
Analogously, is the second parameter of storePrimitive32 (u32 value) guaranteed to have the remaining 10 bits zeroed out?

If it is, I think there is no issue, otherwise masking or shifting should be necessary

testdata/p4tc_samples_outputs/add_entry_1_example_parser.h

testdata/p4tc_samples_outputs/add_entry_3_example_parser.h

vbnogueira · 2024-10-04T18:29:33Z

testdata/p4tc_samples_outputs/add_entry_1_example_control_blocks.c

@@ -114,8 +114,8 @@ if (/* hdr->ipv4.isValid() */
                                break;
                            case MAINCONTROLIMPL_IPV4_TBL_1_ACT_MAINCONTROLIMPL_SEND_NH: 
                                {
-                                    hdr->ethernet.srcAddr = bpf_cpu_to_be64(value->u.MainControlImpl_send_nh.smac);
-                                                                        hdr->ethernet.dstAddr = ntohll(value->u.MainControlImpl_send_nh.dmac << 16);
+                                    storePrimitive64(&hdr->ethernet.srcAddr, 48, (bpf_cpu_to_be64(value->u.MainControlImpl_send_nh.smac)));


I think I accidentally found a bug
Was testing changing EthernetAddress' typecast[1] to a bit<39> and saw that the generated code was outputing this line as:

storePrimitive64(&hdr->ethernet.srcAddr, 40, (bpf_cpu_to_be64((u64)(u64)value->u.MainControlImpl_send_nh.smac)));

From what I understood, the second argument should be 39, not 40

[1] https://github.com/p4lang/p4c/blob/main/testdata/p4tc_samples/add_entry_1_example.p4#L4

I am unable to reproduce this issue, If I change srcaddr to bit<39>, I get error about invalid ethernet packet size.

vbnogueira

I can't show this in the diff because it wasn't changed in this PR [1], however it is causing a compilation error:

1 warning generated.                                                  
/home/vagrant/p4c/backends/tc/runtime/tmpn_esd7rd/add_entry_1_example_control_blocks.c:98:88: error: array type 'u8[6]' (aka 'unsigned char[6]') is not assignable
                                    update_act_bpf_val->u.MainControlImpl_send_nh.dmac = hdr->ethernet.dstAddr;

A similar issue happens in [2]

[1] https://github.com/komaljai/p4c/blob/align_key_bytewise/testdata/p4tc_samples_outputs/add_entry_1_example_control_blocks.c#L96
[2] https://github.com/komaljai/p4c/blob/align_key_bytewise/testdata/p4tc_samples_outputs/add_entry_3_example_control_blocks.c#L96

testdata/p4tc_samples_outputs/add_entry_1_example_control_blocks.c

testdata/p4tc_samples_outputs/add_entry_1_example_parser.h

backends/tc/ebpfCodeGen.cpp

psivanup

Please check the nice to have comments.

psivanup · 2024-11-22T05:21:07Z

backends/ebpf/target.h

@@ -238,6 +238,11 @@ class P4TCTarget : public KernelSamplesTarget {
        }
        return "HOST"_cs;
    }
+
+    bool isPrimitiveByteAligned(int width) const {
+        return (width <= 8 || width <= 16 || (width > 24 && width <= 32) ||


Looks width <= 8 check is redundant

Suggested change

return (width <= 8 || width <= 16 || (width > 24 && width <= 32) ||

return (width <= 16 || (width > 24 && width <= 32) ||

psivanup · 2024-11-22T05:21:34Z

backends/ebpf/ebpfType.h

+
+ public:
+    explicit EBPFScalarTypePNA(const IR::Type_Bits *bits) : EBPFScalarType(bits) {
+        isPrimitiveByteAligned = (width <= 8 || width <= 16 || (width > 24 && width <= 32) ||


Suggested change

isPrimitiveByteAligned = (width <= 8 || width <= 16 || (width > 24 && width <= 32) ||

isPrimitiveByteAligned = (width <= 16 || (width > 24 && width <= 32) ||

psivanup · 2024-11-22T05:27:20Z

backends/ebpf/ebpfType.cpp

+    if (generatesScalar(width) && isPrimitiveByteAligned == true) {
+        builder->append("0");
+    } else {
+        builder->append("{ 0 }");


may be you can add a comment that array initializer is emitted for non byte aligned primitive types as they are emitted as array type.

psivanup · 2024-11-22T05:32:19Z

backends/ebpf/codeGen.cpp

+                                           : "getPrimitive64"_cs;
+                builder->appendFormat("%v((u8 *)", getPrimitive);
+                visit(expression);
+                builder->appendFormat(", %d)", scalar->implementationWidthInBits());


Suggested change

builder->appendFormat(", %d)", scalar->implementationWidthInBits());

builder->appendFormat(", %d)", scalar->implementationWidthInBits());

return;

By adding return to the above code, you can replace all else blocks below with just one visit(expression).

Signed-off-by: Adarsh <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

Co-authored-by: rst0git <[email protected]>

Signed-off-by: Anton Korobeynikov <[email protected]>

Signed-off-by: Komal, Jain <[email protected]>

komaljai changed the title ~~Align key components to the nearest 8-bit size~~ [Draft] - Align key components to the nearest 8-bit size Oct 3, 2024

vbnogueira reviewed Oct 4, 2024

View reviewed changes

komaljai force-pushed the align_key_bytewise branch from 7c87489 to 34b62d4 Compare November 18, 2024 01:10

komaljai changed the title ~~[Draft] - Align key components to the nearest 8-bit size~~ Align key components to the nearest 8-bit size Nov 18, 2024

vbnogueira reviewed Nov 18, 2024

View reviewed changes

komaljai commented Nov 19, 2024

View reviewed changes

backends/tc/ebpfCodeGen.cpp Show resolved Hide resolved

komaljai commented Nov 19, 2024

View reviewed changes

backends/tc/ebpfCodeGen.cpp Show resolved Hide resolved

komaljai force-pushed the align_key_bytewise branch 3 times, most recently from 9635f71 to 295a6ce Compare November 21, 2024 13:15

psivanup approved these changes Nov 22, 2024

View reviewed changes

komaljai force-pushed the align_key_bytewise branch 2 times, most recently from cb20ec1 to e5a365e Compare December 12, 2024 10:27

komaljai force-pushed the align_key_bytewise branch from e5a365e to 889848e Compare December 19, 2024 14:31

AdarshRawat1 and others added 2 commits January 4, 2025 19:07

Bump Doxygen v1.12.0 -> v1.13.0 (p4lang#5080)

52697c9

Signed-off-by: Adarsh <[email protected]>

Teach functions inliner to inline into if condition (p4lang#5073)

057fe94

Signed-off-by: Anton Korobeynikov <[email protected]>

komaljai force-pushed the align_key_bytewise branch 2 times, most recently from bfa6b0c to 998f749 Compare January 6, 2025 09:53

github-actions bot and others added 3 commits January 6, 2025 10:11

Automated Release v1.2.5.2 (p4lang#5081)

e814a33

Co-authored-by: rst0git <[email protected]>

Switch to abseil symbolization / stack tracing routines (p4lang#5077)

80ef07c

Signed-off-by: Anton Korobeynikov <[email protected]>

Align key components to the nearest 8-bit size

ffca7b6

Signed-off-by: Komal, Jain <[email protected]>

komaljai force-pushed the align_key_bytewise branch from 998f749 to ffca7b6 Compare January 7, 2025 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align key components to the nearest 8-bit size #30

Align key components to the nearest 8-bit size #30

komaljai commented Oct 3, 2024

vbnogueira left a comment •

edited

Loading

vbnogueira Oct 4, 2024

komaljai Nov 21, 2024

vbnogueira left a comment

psivanup left a comment

psivanup Nov 22, 2024

psivanup Nov 22, 2024

psivanup Nov 22, 2024

psivanup Nov 22, 2024

	return (width <= 8 \|\| width <= 16 \|\| (width > 24 && width <= 32) \|\|
	return (width <= 16 \|\| (width > 24 && width <= 32) \|\|

	isPrimitiveByteAligned = (width <= 8 \|\| width <= 16 \|\| (width > 24 && width <= 32) \|\|
	isPrimitiveByteAligned = (width <= 16 \|\| (width > 24 && width <= 32) \|\|

	builder->appendFormat(", %d)", scalar->implementationWidthInBits());
	builder->appendFormat(", %d)", scalar->implementationWidthInBits());
	return;

Align key components to the nearest 8-bit size #30

Are you sure you want to change the base?

Align key components to the nearest 8-bit size #30

Conversation

komaljai commented Oct 3, 2024

vbnogueira left a comment • edited Loading

Choose a reason for hiding this comment

vbnogueira Oct 4, 2024

Choose a reason for hiding this comment

komaljai Nov 21, 2024

Choose a reason for hiding this comment

vbnogueira left a comment

Choose a reason for hiding this comment

psivanup left a comment

Choose a reason for hiding this comment

psivanup Nov 22, 2024

Choose a reason for hiding this comment

psivanup Nov 22, 2024

Choose a reason for hiding this comment

psivanup Nov 22, 2024

Choose a reason for hiding this comment

psivanup Nov 22, 2024

Choose a reason for hiding this comment

vbnogueira left a comment •

edited

Loading