Skip to content

Explicit packing of Command/Status - #57

Open
quetric wants to merge 3 commits into
definelicht:masterfrom
quetric:explicit_packing
Open

Explicit packing of Command/Status#57
quetric wants to merge 3 commits into
definelicht:masterfrom
quetric:explicit_packing

Conversation

@quetric

@quetric quetric commented Dec 15, 2021

Copy link
Copy Markdown

This PR has two changes:

  • added bytesReceived and endOfPacket fields to hlslib::axi::Status which are set by datamovers when Indeterminate BTT mode is set
  • added constructors and casting functions enabling easy explicit conversion of Command/Status to/from ap_uint. There appears to be a problem with tight packing of structure fields on stream interfaces in the latest Vitis HLS, therefore this change makes it easier to do manual packing.

@definelicht

Copy link
Copy Markdown
Owner

Thanks Lucian -- you didn't change the internal representation of the objects, doesn't that mean that they will still not be tightly packed in the HLS code? I was thinking something like this:

class Status {
   Status(ap_uint<4> tag, ap_uint<1> internalError, ap_uint<1> decodeError, ap_uint<1> slaveError, 
          ap_uint<1> okay, ap_uint<23> bytesReceived, ap_uint<1> endOfPacket) {
    data_(3,0) = tag;
    data_(4,4) = internalError;
    data_(5,5) = decodeError;
    data_(6,6) = slaveError;
    data_(7,7) = okay;
    data_(30,8) = bytesReceived;
    data_(31,31) = endOfPacket;
  }
  ap_uint<4> tag() const {
    return data_(3, 0);
  }
  // Repeat for all fields...
 private:
  ap_uint<32> data_;
}

Wouldn't this guarantee tight packing?

@quetric

quetric commented Dec 16, 2021

Copy link
Copy Markdown
Author

I didn't think of changing the underlying storage tbh. The way i used the code in this PR is to have ap_uint streams on interfaces and convert to and from Status/Command for internal use. Something like:

void err_forward(hls::stream<ap_uint<32> > &status, hls::stream<ap_uint<32> > &error){
    hlslib::axi::Status sts = hlslib::axi::Status(status.read());
    if(!sts.okay) error.write(sts);
} 

The result is tightly packed because ap_uint is what's streamed. But your solution is better because it allows the Status/Command to be carried directly on the stream. I can give it a try in a few days, unless you want to make the change yourself.

@definelicht

definelicht commented Dec 16, 2021

Copy link
Copy Markdown
Owner

Then I like my solution more, if it works. But the critical thing to check is that the size of the struct containing the ap_uint is equal to the naked ap_uint, also for non-byte aligned structs. No rush from my side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants