POETSII / tinsel

Manythread RISC-V overlay for FPGA clusters
Other
35 stars 1 forks source link

Truncation of tinselUartTryPut CSR value #39

Closed m8pple closed 6 years ago

m8pple commented 6 years ago

When using the CSR_TO_UART register which underlies tinselUartTryPut, will the 32-bit value be truncated to the 8 LSBs in hardware, or does it need to be masked?

Only the 8-bits will turn up in software, but is there any problem if there is non-zero junk in the top 24-bits written to the CSR? My assumption is that it's fine, but I just wondered because tinselUartTryPut takes a uint8_t argument, which would effectively zero out the top three bytes.

It is on the inner loop of a few logging functions, so would decrease code size for logging if I can drop the mask instructions before writing the CSR.

mn416 commented 6 years ago

Hi David,

Yep, no need to do a mask, only the bottom 8 bits are used by the hardware:

https://github.com/POETSII/tinsel/blob/master/rtl/Core.bsv#L851

mn416 commented 6 years ago

If the uint8_t is causing extra instructions to be generated then we should change the type to uint32_t. That's a good point.

m8pple commented 6 years ago

The uint8_t does make sense on the standard API from a signature-as-documentation point of view, as it makes clear to the programmer that only 8-bits can be sent.

TBH I didn't check that an extra instruction would be generated, but I assumed it must be to obey C semantics, unless the compiler could statically prove at the inlining site that the upper 24-bits were zero.

mn416 commented 6 years ago

Agreed, I think the cast to uint8_t and then back to uint32_t will require a mask. This will require an extra instruction at every call site due to inlining. Happy to add a variant that takes a uint32_t to reduce instruction memory usage, if you like (or you can make your own).

m8pple commented 6 years ago

I'd say leave the public API as it is - anyone relying on low-level details is going to be architecture dependent anyway, so they should probably be checking what is going on at a low level.