add axistream library by stephenneuendorffer · Pull Request #3 · Xilinx/HLS_packet_processing

stephenneuendorffer · 2019-11-25T23:08:54Z

For review.

stephenneuendorffer

Thanks for this. I think there's some things which you've defined in a more elegant way.. Perhaps we could have a phone call to see about how to proceed? I wasn't expecting something which is essentially a standalone redesign.

stephenneuendorffer · 2019-11-25T23:12:34Z

+## Building
+The tests require [Catch2](https://github.com/catchorg/Catch2).
+Download `catch.hpp`, and then set the `CATCH_DIR` variable in the `Makefile`
+accordingly.
+
+Configure the following 2 lines the in Makefile:
+```
+CATCH_DIR = ../catch
+HLS_INCLUDE_DIR = /u/tsqahfus/local/tools/xilinx/Vivado/2018.3/include/
+```


Perhaps this could be a git submodule?

stephenneuendorffer · 2019-11-25T23:13:28Z

+ * Simple array type for use in HLS.
+ * This serves as a translatable version of std::array,
+ * for cases when an array with value semantics is needed.
+ * (As opposed to normal C arrays which decay to pointers.)
+ */


I'm surprised std::array is not directly synthesizable? I guess it contains malloc?

std::array doesn't contain malloc, there actually is no reason it shouldn't be synthesizable. It's just an array: https://gcc.gnu.org/onlinedocs/gcc-9.2.0/libstdc++/api/a00041_source.html.

I actually never tried synthesizing it before; I just assumed it wouldn't work. I tried it today though and it works fine!

nice! seems worth pulling that out then.

stephenneuendorffer · 2019-11-25T23:16:01Z

+        ap_uint<util::unsigned_bit_width<work_size - 1>::value> last_index = DataWidth - len_ + MsgWidth - 1;
+
+        for (int i = 0; i < max_beats; i++) {
+            HLS_PRAGMA(HLS unroll);


This unroll is somewhat problematic. It's somewhat a tradeoff whether this should be a pipelined loop or not. It depends a bit on the context in which you expect the code to be used. What examples have you built?

I need to test this more. The interesting thing about unrolling this loop is that if you read multiple messages in sequence, and all the read calls are inlined and this loop is unrolled, the offset for each field in the stream is statically known. As for being a pipelined loop, it has an interation latency of 1 cycle anyway.

I agree, there is a huge value in making the offsets statically known. There are actually several places where this comes in. The simplest one is ensuring that offsets in a header are statically known. If they are not statically known, then narrowing them down to a small set of options is important (offset is either 64 or 96, but it's not arbitrary). The second one is ensuring that the offset within the width of the AXI stream where data starts is constant. I suspect that unrolling this loop primarily helps with having constant offsets in a header. It also helps reduce the loop overhead generated by HLS. (this is more of a limitation in HLS, since max_beats is often very small for interesting packet formats, which means pipelining this loop can have significant performance impacts.

It looks like you've focused on the model of accessing one header at a time. Did you consider the model of operating on a whole packet at a time?

I've kind of left that till the next layer - this code just reads a message (a single header) from a stream. Then if someone wants a whole packet, they'd call read to get a bunch of headers:

ethernet_header e = reader.read<ethernet_header>(); ip_header ip = reader.read<ip_header>(); // ...

If after they read the headers, they want the data, then they could just call read_raw<8>, for example, to get 8 bytes at a time. And if they want to store the data in a RAM, they do that themselves:

char data[100]; for (int i = 0; i < len; i++) { data[i] = reader.read_raw<1>(); }

But if they want to process the data and send it back out at the same time, they just don't store it, but still loop through it.
Then the user does whatever they want with the data later.

Also, this unroll was based on:

HLS_packet_processing/apps/common/ip.hpp

Lines 2849 to 2851 in efbd999

for(int i = 0; i < (S+DATA_S-1)/DATA_S; i++)

#pragma HLS unroll

if (t_bytesBuffered < S) {

The code to get the ending condition is tricky while writing it in a width-parameterizable way. This is one reason why I like the 'packet' model for writing the code.

stephenneuendorffer · 2019-11-25T23:16:45Z

+template <int DataWidth>
+class AxiStreamReader {
+public:
+    AxiStreamReader(AxiStream<DataWidth>& stream) : stream_(stream) {


Do you think it is possible to make this a drop-in replacement for the existing classes?

Yes. It roughly is now; the only difference are the method names (get vs read, etc): https://github.com/Xilinx/HLS_packet_processing/blob/master/apps/common/ip.hpp#L2811

Would it make sense to modify the existing class to take a message as another option? This could make it easier to do an apples-to-apples comparison.

Then we could just see the difference of the parsing code. It's probably possible to do that (at least in some hacky temporary way), the key is this set_le call: https://github.com/Xilinx/HLS_packet_processing/blob/master/apps/common/ip.hpp#L2869. It also expects a LENGTH member: https://github.com/Xilinx/HLS_packet_processing/blob/master/apps/common/ip.hpp#L2829. But you could make a message conform to those requirements, and the LittleEndianByteReader should work I think.

It might be easier to try both systems on say an Ethernet + ipv4 header independently and just see the difference.

I think both are worthwhile to understand the differences. I'm loathe to give up proper handling of KEEP/LAST. I like your message abstraction and I think it could make the code simpler for defining packet formats.

I have a version which handles keep/last correctly (prevents you from running off the end) that I'm experimenting with; once I get it working I'll add it.

I'm trying to make it parameterizable on how it handles length checking, because once you do keep/last correctly, the loop exit condition (or the if inside the loop) is data-dependent, which seems to increase the iteration latency of the loop, but I'm still experimenting.

I think that bit is well handled in the original code. I suspect you're just going to end up recreating what is there. I'd like to suggest focusing on adapting those classes to support your message type.

stephenneuendorffer · 2019-11-25T23:17:00Z

+class AxiStreamWriter {
+public:
+    AxiStreamWriter(AxiStream<DataWidth>& stream) : stream_(stream) {
+        HLS_PRAGMA(HLS inline);
+        reset();


drop-in replacement?

Same, only difference are the method names: https://github.com/Xilinx/HLS_packet_processing/blob/master/apps/common/ip.hpp#L3006

stephenneuendorffer · 2019-11-25T23:20:08Z

@@ -0,0 +1,58 @@
+#include "catch.hpp"


I like all the tests. :)

stephenneuendorffer · 2019-11-25T23:24:22Z

+ * This struct can be represented in the message framework as:
+ * struct A : Int<64, Sign::UNSIGNED, Endianness::LITTLE> {};
+ * struct B : Int<32, Sign::SIGNED, Endianness::LITTLE> {};
+ * using MyStruct = Struct<A, B>;
+ *


OK, so fundamentally, this seems very similar to the old technique. A field of a header is represented using a named type. You're just using variadic templates directly to make things work, instead of mpl::vector<>?

Right - this was all inspired by your original design. Using the variardic template instead of mpl::vector is one key difference. Another is using inheritance to define fields, instead of using mpl::string.

There is also a richer message type system now. You can have nesting, for example:

struct Field1 : MyStruct {}; struct Field2 : MyStruct {}; using MyStructPair = Struct<Field1, Field2>;

Enums are also supported, as are unions.

stephenneuendorffer · 2019-11-25T23:30:47Z

+struct Int {
+    static const int width = W;
+
+    typedef ap_uint<width> raw;


I'd suggest that the raw bits should be private and only visible to the user in an endian-agnostic way.

👍 I'll try that.

stephenneuendorffer · 2019-11-25T23:31:33Z

+    typedef ap_uint<width> raw;
+    typedef typename TypeSelector<W, S>::type type;


OK, so one advantage of this is that you have the ability to define arbitrary width fields. Somewhat purposely, I had limited things in the original library to being 8-bit granularity. One reason for this is that muxing gets expensive, and I wanted to ensure that the muxing wasn't hugely expensive for very wide data paths, even if constant offsets could not be proven.

Within a message, every field has a constant offset - the offset is computed at compile time via the templates. So reading a field from a message should be a nop - a field is a named slice.

stephenneuendorffer · 2019-11-25T23:31:40Z

+using i8 = li<8>;
+using li16 = li<16>;
+using li32 = li<32>;
+using li64 = li<64>;
+using bi16 = bi<16>;
+using bi32 = bi<32>;
+using bi64 = bi<64>;
+
+using u8 = lu<8>;
+using lu16 = lu<16>;
+using lu32 = lu<32>;
+using lu64 = lu<64>;
+using bu16 = bu<16>;
+using bu32 = bu<32>;
+using bu64 = bu<64>;


I tried to avoid defining specific descriptors like this. Although I don't think it's super difficult to do it this way, I was hoping to have people define their header structures just using regular types.

I tried to figure out how to do that, but it just worked better if everything was in this system.

In the original library, you just used newfield right? https://github.com/Xilinx/HLS_packet_processing/blob/master/apps/common/ip.hpp#L2182:

typedef newfield<ap_uint<16>, boost::mpl::string<'type'> > etherType;

Or was there another way?

This is fundamentally the same as what you are doing. Lookup is by type comparison in a sequence of types. I agree that it's probably simpler the way you've defined these types, rather than using boost::mpl, which is a significant library dependance and also has some syntactic overhead.

stephenneuendorffer · 2019-11-26T23:52:46Z

+#pragma once
+
+#pragma GCC system_header
+#include "ap_int.h"


-Wno-unknown-pragmas ?

stephenneuendorffer · 2019-11-27T00:00:01Z

+        ap_uint<util::unsigned_bit_width<work_size - 1>::value> last_index = DataWidth - len_ + MsgWidth - 1;
+
+        for (int i = 0; i < max_beats; i++) {
+            HLS_PRAGMA(HLS unroll);


The code to get the ending condition is tricky while writing it in a width-parameterizable way. This is one reason why I like the 'packet' model for writing the code.

stephenneuendorffer · 2019-11-27T00:07:54Z

+ * This standard differs from the ARM standard
+ * (https://static.docs.arm.com/ihi0051/a/IHI0051A_amba4_axi4_stream_v1_0_protocol_spec.pdf)
+ * in the following key ways:
+ * TKEEP: is ignored, since null bytes are only used in a packet with TLAST set, and the user of this API requests a packet by length.


I expect that many practical packet formats are not of known size. I think handling Keep and Last correctly is important. Are all the formats you need fixed-size packets?

stephenneuendorffer · 2019-11-27T00:09:55Z

+template <int DataWidth>
+class AxiStreamReader {
+public:
+    AxiStreamReader(AxiStream<DataWidth>& stream) : stream_(stream) {


I think both are worthwhile to understand the differences. I'm loathe to give up proper handling of KEEP/LAST. I like your message abstraction and I think it could make the code simpler for defining packet formats.

orangeturtle739

New version, with mold example & synthesis reports.

orangeturtle739 · 2019-12-10T15:25:19Z

+#include "moldudp64.hh"
+#include "itch.hh"
+
+void mold_example(AxiStream<8>& in_stream, hls::stream<int32_t>& out) {


@stephenneuendorffer I made the mold_remover_stream example:

HLS_packet_processing/apps/mold_remover_stream/app.cpp

Lines 36 to 125 in efbd999

void process_packet(ap_uint<48> macAddresst, ap_uint<32> ipAddress, int port,

hls::stream<StreamType> &input,

hls::stream<StreamType> &output,

hls::stream<ap_uint<32> > &outputMeta) {

MACAddressT macAddress = macAddresst;

#pragma HLS inline all recursive

// Define the structure of the packets we're interested in. Generally speaking we

// have parallel access to the 'header' portion (the values are stored in registers),

// and sequential access to the 'packet' portion (the values are stored in BRAM).

auto reader = make_reader(input);

auto writer = make_writer(output);

// Read the headers from the input stream.

ethernet::header eh;

reader.get(eh);

ipv4::header ih;

reader.get(ih);

ipv4::udp_header uh;

reader.get(uh);

// Check that the packet is something we care about.

ap_uint<16> dmp_macType = eh.get<ethernet::etherType>();

bool valid = true;

MACAddressT destinationMAC = eh.get<ethernet::destinationMAC>();

if(destinationMAC != macAddress &&

destinationMAC != BROADCAST_MAC) {

#ifndef __SYNTHESIS__

std::cout << "Dropping invalid MAC address: " << macAddress.toString(16, false) << " " << destinationMAC.toString(16, false) << "\n";

// hexdump_ethernet_frame<4>(buf, len);

#endif

valid = false;

}

if(eh.get<ethernet::etherType>() != ethernet::ethernet_etherType::IPV4) {

valid = false;

}

if(ih.get<ipv4::protocol>() != ipv4::ipv4_protocol::UDP) {

valid = false;

}

if(ih.get<ipv4::destination>() != ipAddress) {

valid = false;

}

if(uh.get<ipv4::dport>() != port) {

valid = false;

}

if(valid) {

mold64::header mh;

reader.get(mh);

mold_message_loop:

// Iterate over each mold message, starting at offset 0 after the mold header.

for(int i = 0; i < mh.get<messageCount>(); i++) {

// Extract a mold message from the 'packet' portion

mold64::message_header mold_message;

reader.get(mold_message);

// Look at the mold message to figure out how long the itch message is.

int length = mold_message.get<messageLength>();

// Extract the itch message and do something with it.

// "mold_message.p" is the 'rest' of the packet after the mold message header

// Parsing a packet is a constant time operation and the data is not moved.

// Getting the fields of a 'parsed' header in this case is sequential, since it

// was originally deserialized into a Packet object.

itch::header itch_message;

//auto itch_message = itch::header::contains(message);

//itch_message.extend(length);

reader.get(itch_message);

//std::cout << "message:" << (char)itch_message.get<messageType>() << "\n";

if(itch_message.get<messageType>() == 'R') {

itch::directory_header m;

reader.read_rest();//

//reader.get(m);

//auto itch_directory_message = parse_itch_directory_hdr(message);

// itch_directory_message.serialize(output);

outputMeta << 1;

} else if(itch_message.get<messageType>() == 'A') {

itch::add_order_header m;

//reader.read_rest();//

reader.get(m);

//auto itch_add_order_message = parse_itch_add_order_hdr(message);

// itch_add_order_message.serialize(output);

outputMeta << 2;

}

}

} else {

// Dump the remaining packet

reader.read_rest();

}

}

orangeturtle739 · 2019-12-10T15:25:54Z

+        N/A
+
+        * Loop: 
+        +---------------------+-----+--------+----------+-----------+-----------+-----------+----------+


My implementation: loop is 4-6 cycles, vs 5-7 on the old implementation.

orangeturtle739 · 2019-12-10T15:27:13Z

+|Multiplexer      |        -|      -|        -|      326|    -|
+|Register         |        -|      -|     1001|        -|    -|
+-----------------+---------+-------+---------+---------+-----+
+|Total            |        0|      0|     1001|     1222|    0|


1222 luts vs. 1226 luts, but 1001 FFs vs 428.

orangeturtle739 · 2019-12-10T15:27:50Z

@@ -0,0 +1,238 @@
+
+
+================================================================


Report for your example, with the same target part and clock.

orangeturtle739 · 2019-12-10T15:29:22Z

+            // Using received_upper_bound here instead of received helps the HLS tool
+            // because then it is evident that the loop only exits on last_,
+            // and not the uncontrained output of compute_received_delta.
+			if (received_upper_bound >= Received(MsgWidth) || !LengthPolicy::more_available(last_)) {


The LengthPolicy enables Checked and Unchecked reads. Checked reads respect keep & last, and return both the data read, and the number of bytes actually read.

orangeturtle739 · 2019-12-10T15:36:31Z

+#pragma once
+
+#pragma GCC system_header
+#include "ap_int.h"


Here are the errors I get if I remove these system_header pragmas: https://gist.github.com/orangeturtle739/ea24d2d937ba37300f5607502f9962cc.

orangeturtle739 · 2019-12-10T15:37:23Z

+ */
+
+
+#pragma once


These are all pretty minimal, just to support the example.

orangeturtle739 · 2019-12-10T15:38:23Z

+    CHECK(result.len == 1);
+
+    // Check that at the end of the packet, we can't read anymore until a drain()
+    result = reader.read_raw<LengthPolicy::Checked, 16>();


Here are the checked reads, which keep track of keep and last.

orangeturtle739 · 2019-12-10T15:39:18Z

            if(itch_message.get<messageType>() == 'R') {
                itch::directory_header m;
-                reader.read_rest();//
+                // reader.read_rest();//


The other branch of the if had drain commented out.

orangeturtle739 · 2019-12-10T15:39:33Z

+# set_part {xc7z020clg484-1}
+# create_clock -period 5.000000 -name default
+set_part {xcvu9p-flgb2104-3-e}
+create_clock -period 3.103 -name default


Use the same part & clock I tried my example on.

sthornington · 2023-01-29T15:17:55Z

Is this PR dead?

add axistream library

f29e357

stephenneuendorffer commented Nov 25, 2019

View reviewed changes

stephenneuendorffer commented Nov 27, 2019

View reviewed changes

orangeturtle739 added 2 commits December 10, 2019 10:15

add mold example

3f774ef

make mold_remover_stream example equivelent

70d76ef

orangeturtle739 reviewed Dec 10, 2019

View reviewed changes

	for(int i = 0; i < (S+DATA_S-1)/DATA_S; i++)
	#pragma HLS unroll
	if (t_bytesBuffered < S) {

		typedef ap_uint<width> raw;
		typedef typename TypeSelector<W, S>::type type;

	void process_packet(ap_uint<48> macAddresst, ap_uint<32> ipAddress, int port,
	hls::stream<StreamType> &input,
	hls::stream<StreamType> &output,
	hls::stream<ap_uint<32> > &outputMeta) {
	MACAddressT macAddress = macAddresst;
	#pragma HLS inline all recursive
	// Define the structure of the packets we're interested in. Generally speaking we
	// have parallel access to the 'header' portion (the values are stored in registers),
	// and sequential access to the 'packet' portion (the values are stored in BRAM).

	auto reader = make_reader(input);
	auto writer = make_writer(output);

	// Read the headers from the input stream.
	ethernet::header eh;
	reader.get(eh);
	ipv4::header ih;
	reader.get(ih);
	ipv4::udp_header uh;
	reader.get(uh);

	// Check that the packet is something we care about.
	ap_uint<16> dmp_macType = eh.get<ethernet::etherType>();
	bool valid = true;
	MACAddressT destinationMAC = eh.get<ethernet::destinationMAC>();
	if(destinationMAC != macAddress &&
	destinationMAC != BROADCAST_MAC) {
	#ifndef __SYNTHESIS__
	std::cout << "Dropping invalid MAC address: " << macAddress.toString(16, false) << " " << destinationMAC.toString(16, false) << "\n";
	// hexdump_ethernet_frame<4>(buf, len);
	#endif
	valid = false;
	}
	if(eh.get<ethernet::etherType>() != ethernet::ethernet_etherType::IPV4) {
	valid = false;
	}
	if(ih.get<ipv4::protocol>() != ipv4::ipv4_protocol::UDP) {
	valid = false;
	}
	if(ih.get<ipv4::destination>() != ipAddress) {
	valid = false;
	}
	if(uh.get<ipv4::dport>() != port) {
	valid = false;
	}
	if(valid) {
	mold64::header mh;
	reader.get(mh);
	mold_message_loop:
	// Iterate over each mold message, starting at offset 0 after the mold header.
	for(int i = 0; i < mh.get<messageCount>(); i++) {
	// Extract a mold message from the 'packet' portion
	mold64::message_header mold_message;
	reader.get(mold_message);

	// Look at the mold message to figure out how long the itch message is.
	int length = mold_message.get<messageLength>();

	// Extract the itch message and do something with it.
	// "mold_message.p" is the 'rest' of the packet after the mold message header
	// Parsing a packet is a constant time operation and the data is not moved.
	// Getting the fields of a 'parsed' header in this case is sequential, since it
	// was originally deserialized into a Packet object.
	itch::header itch_message;
	//auto itch_message = itch::header::contains(message);
	//itch_message.extend(length);
	reader.get(itch_message);
	//std::cout << "message:" << (char)itch_message.get<messageType>() << "\n";
	if(itch_message.get<messageType>() == 'R') {
	itch::directory_header m;
	reader.read_rest();//
	//reader.get(m);
	//auto itch_directory_message = parse_itch_directory_hdr(message);
	// itch_directory_message.serialize(output);
	outputMeta << 1;
	} else if(itch_message.get<messageType>() == 'A') {
	itch::add_order_header m;
	//reader.read_rest();//
	reader.get(m);
	//auto itch_add_order_message = parse_itch_add_order_hdr(message);
	// itch_add_order_message.serialize(output);
	outputMeta << 2;
	}
	}
	} else {
	// Dump the remaining packet
	reader.read_rest();
	}
	}

+                      N/A
+                      * Loop:
+                      +---------------------+-----+--------+----------+-----------+-----------+-----------+----------+

		@@ -0,0 +1,238 @@


		================================================================

Uh oh!

Conversation

stephenneuendorffer commented Nov 25, 2019

Uh oh!

stephenneuendorffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orangeturtle739 Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

orangeturtle739 Nov 26, 2019 •

edited

Loading

orangeturtle739 Dec 10, 2019 •

edited

Loading