Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicit packing of Command/Status #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

quetric
Copy link

@quetric quetric commented Dec 15, 2021

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
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
Copy link
Author

quetric commented Dec 16, 2021

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
Copy link
Owner

definelicht commented Dec 16, 2021

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.

None yet

2 participants