search

firesim / firesim

FireSim: Fast and Effortless FPGA-accelerated Hardware Simulation with On-Prem and Cloud Flexibility
https://fires.im
Other
869 stars 229 forks source link

HostPort abuses some Chisel APIs #653

Open jackkoenig opened 3 years ago

jackkoenig commented 3 years ago

Related PR in chisel3 https://github.com/freechipsproject/chisel3/pull/1654. HostPort doesn't clone its input Data which makes it really easy to violate certain assumptions in Chisel. There's also use of unbound Records that can be problematic: https://github.com/firesim/firesim/blob/c2d8e3a46e59222e115a1fdaa7267592e1d3c503/sim/firesim-lib/src/main/scala/bridges/UARTBridge.scala#L44

davidbiancolin commented 3 years ago

What are the potential side effects of having an unbound record? Can they be elided if the record is not assigned to a val?

jackkoenig commented 3 years ago

What are the potential side effects of having an unbound record?

Currently, every Data has a pointer to its parent module and the parent module has a pointer to the Data. I intend to fix this implementation detail in https://github.com/freechipsproject/chisel3/pull/1624.

Can they be elided if the record is not assigned to a val?

No, by virtue of existing it trips the current bug

Fundamentally, while the pattern of unbound Data having references to other Data is awkward it's not fundamentally wrong (thus https://github.com/freechipsproject/chisel3/pull/1654). But trying to bind the Data is an error, and the pattern might lend itself to people trying to extend it or copy and tweak and it not being able to do what they expect.

jackkoenig commented 3 years ago

I put a link in that PR to this issue, but there's a much more thorough discussion of the problem here: https://github.com/freechipsproject/chisel3/pull/1655

Fundamentally, having a Data be a part of multiple different Aggregates violates a ton of assumptions in Chisel. These assumptions are not well enforced (thus my calling it an API hole in the PR linked above), but it can lead to some weird corner cases.

Some refactoring of the internals of Chisel (unifying the parallel bookkeeping of binding and refs) can better support your use case here, but the behavior of HostPortIO not cloning it's element in its own cloneType is an API hole that needs to be closed in Chisel 3.5 in my opinion.