Align key components to the nearest 8-bit size

[komaljai]

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

[vbnogueira]

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

[komaljai]

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

[psivanup]

Please check the nice to have comments.

[psivanup]

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]

Suggested change

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

[psivanup]

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]

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).