search

rdaly525 / coreir

BSD 3-Clause "New" or "Revised" License
100 stars 24 forks source link

Add support for Verilog ndarray codegen #928

Closed leonardt closed 4 years ago

leonardt commented 4 years ago

I tried to implement this using a flag to guard the new behavior, but it seemed to difficult without having duplicated code paths. The main change was that the ConnectionMap now maps using a vector index instead of an integer index (since we can't assume it's a 1d index). Then, the array connection logic was updated to recursively construct concat nodes (if it's a multi-dimensional array, then the concats will be generated for each dimension.

This should be backwards compatible with the existing 1d array support (modulo any implementation bugs), so there's no interface change here.

leonardt commented 4 years ago

CC @rsetaluri just FYI, in this PR, I've updated the codegen logic to assign instance inputs to wires, then just have the module instance statement only emit wire names (e.g. Foo foo(.I(Foo_I), ...)). This was requested for tool compatibility and I thought was required by verilator for the ndarray support (turns out it was a different issue). I'll leave it in for now since it's been previously requested, but let me know (once this is release or if you have a chance to early test) if this causes problems with your PD issues like the mantle wires did. The useful thing is that it doesn't introduce a wire module/instance, so maybe the tool won't have a problem handling it? (It just introduces a wire statement and an assign)

rsetaluri commented 4 years ago

CC @rsetaluri just FYI, in this PR, I've updated the codegen logic to assign instance inputs to wires, then just have the module instance statement only emit wire names (e.g. Foo foo(.I(Foo_I), ...)). This was requested for tool compatibility and I thought was required by verilator for the ndarray support (turns out it was a different issue). I'll leave it in for now since it's been previously requested, but let me know (once this is release or if you have a chance to early test) if this causes problems with your PD issues like the mantle wires did. The useful thing is that it doesn't introduce a wire module/instance, so maybe the tool won't have a problem handling it? (It just introduces a wire statement and an assign)

Ok that makes sense. Thanks for the heads up.

rsetaluri commented 4 years ago

CC @rsetaluri just FYI, in this PR, I've updated the codegen logic to assign instance inputs to wires, then just have the module instance statement only emit wire names (e.g. Foo foo(.I(Foo_I), ...)). This was requested for tool compatibility and I thought was required by verilator for the ndarray support (turns out it was a different issue). I'll leave it in for now since it's been previously requested, but let me know (once this is release or if you have a chance to early test) if this causes problems with your PD issues like the mantle wires did. The useful thing is that it doesn't introduce a wire module/instance, so maybe the tool won't have a problem handling it? (It just introduces a wire statement and an assign)

Ok that makes sense. Thanks for the heads up.

@leonardt on this note however, what is the long term desired thing? Do we want these to be inlined as well (and it's not for backwards compatability?) Either way, ostensibly, we could have switches for all these different codegen options. It's been something on the roadmap, so let me gauge interest for that level of codegen customizability.

leonardt commented 4 years ago

I don't think we want them to be inlined. At FB, when we first introduced the inline logic, Nirav commented that in general it's better to keep expressions out of module instance statements (wire them up to a wire and just use an identifier instead) for the best tool compatibility. Now, FWIW we haven't actually run into any compatibility issues yet so it's not a high priority change (I thought I was running into this issue with verilator which is why I did it, but it ended up being a different syntax issue). It's easy enough to revert this change if it causes issues with the aha PD flow, or make it an option.

I don't think it really affects readability too much.

rsetaluri commented 4 years ago

I don't think we want them to be inlined. At FB, when we first introduced the inline logic, Nirav commented that in general it's better to keep expressions out of module instance statements (wire them up to a wire and just use an identifier instead) for the best tool compatibility. Now, FWIW we haven't actually run into any compatibility issues yet so it's not a high priority change (I thought I was running into this issue with verilator which is why I did it, but it ended up being a different syntax issue). It's easy enough to revert this change if it causes issues with the aha PD flow, or make it an option.

I don't think it really affects readability too much.

That makes sense. I think the crux of the PD issue was the module not the wires so hopefully this should be fine.

leonardt commented 4 years ago

In the process of trying to debug an issue, I discovered that ncsim doesn't allow the unpacked array concat to be inside a module instance statement (see https://buildkite.com/stanford-aha/aha-flow/builds/2524#b69758a3-17cc-45e4-8bce-c4a366e4270a/3379-3594), so we'll need this change for tool support (assign inputs on a separate line)

leonardt commented 4 years ago

Now passing the flow: https://github.com/hofstee/aha/pull/754, so I'll take that as a green light to merge since that's probably one of our most complicated tests.

I'll do a quick review of the code and maybe add some more comments then merge this + all the downstream tools (magma, mantle, fault)