hlslib icon indicating copy to clipboard operation
hlslib copied to clipboard

Explicit packing of Command/Status

Open quetric opened this issue 3 years ago • 3 comments

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.

quetric avatar Dec 15 '21 18:12 quetric

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?

definelicht avatar Dec 16 '21 09:12 definelicht

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.

quetric avatar Dec 16 '21 11:12 quetric

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!

definelicht avatar Dec 16 '21 13:12 definelicht