More of the bindings refactor

[ducky64]

Rest of the Bindings refactor

• Separate out initialization constructors, with deprecation:
  • Wire(init=...) => WireInit
  • Vec(elts) => VecInit
• Require Vec, Wire, and Mem constructors to take a chisel type, not a hardware type.
• Fixes to test suite to make it pass, because it was creating Wires from hardware types

Tests with rocket-chip pending, will fix the use cases in chisel test suite soon so they stop spewing deprecated warnings.

It's likely that rocket-chip will need some fixes to match the new semantics. That list to come soon...

Part 2 (of probably 3?) in a series of PRs for #578

[ducky64]

rocket builds on this now, with one change to src/main/scala/uncore/axi4/Deinterleaver.scala

diff --git a/src/main/scala/uncore/axi4/Deinterleaver.scala b/src/main/scala/uncore/axi4/Deinterleaver.scala
index 3778c91..e7cb257 100644
--- a/src/main/scala/uncore/axi4/Deinterleaver.scala
+++ b/src/main/scala/uncore/axi4/Deinterleaver.scala
@@ -46,7 +46,7 @@ class AXI4Deinterleaver(maxReadBytes: Int)(implicit p: Parameters) extends LazyM
         val qs = Seq.tabulate(endId) { i =>
           val depth = edgeOut.master.masters.find(_.id.contains(i)).flatMap(_.maxFlight).getOrElse(0)
           if (depth > 0) {
-            Module(new Queue(out.r.bits, beats)).io
+            Module(new Queue(out.r.bits.cloneType, beats)).io
           } else {
             Wire(new QueueIO(out.r.bits, beats))
           }

[edwardcwang]

Why is the clonetype needed?

[ducky64]

The new semantics require a chisel type on the Mem constructor in chisel3._. What happens here is that rocket invokes a Queue constructor (under Chisel._) passing in a hardware type, but the Queue internals are chisel3._ which requires the stricter semantics.

This is a larger problem in the compatibility layer, since many things are only validated at the chiselFrontend level (UInt(...), Reg(...), ...), but a higher-level library (like chisel3.util) may be passing arguments down but not validating them or passing through compile options.

[edwardcwang]

Is it possible to introduce an overload for Queue that takes in a hardware type and calls clonetype to get the Chisel type automatically?

[jackkoenig]

Even if it leads to a little code duplication we really need to not force Chisel._ users to change their code

[ducky64]

I could patch Queue specifically, but the more general problem here is that libraries (like chisel utils) are loose in what they accept and don't propagate (or even take) compile options. I'm not sure as to the best way to solve this.

Also, I think a fix in rocket is fine since it's only one use, rather than generally accepted practice (as in, I think the convention people are following is to pass in chisel types where chisel types are expected, and this was an instance where a break in convention was not caught and made it through.

[edwardcwang]

I could patch Queue specifically, but the more general problem here is that libraries (like chisel utils) are loose in what they accept and don't propagate (or even take) compile options. I'm not sure as to the best way to solve this.

What other libraries in utils have the same issue, and does it make sense to pre-emptively work around them (e.g. deal with compatibility errors like chisel vs hardware type)?

Also, I think a fix in rocket is fine since it's only one use, rather than generally accepted practice (as in, I think the convention people are following is to pass in chisel types where chisel types are expected, and this was an instance where a break in convention was not caught and made it through.

Is this convention valid in Chisel._-land though?

[ducky64]

It's an issue that potentially affects everything in utils, but this seems to be the only problem in rocket.

This PR doesn't add strict semantics to Bundle construction, though. That might cause more issues, and a more robust workaround may be needed.

[ducky64]

Resolution: add CompileOptions to Queue and hope that this is the only code that this change breaks, also add a big nasty dynamic deprecation warning when this is detected

[ducky64]

@jackkoenig does this fix things?
(it seems the original rocket-chip breakage has been addressed upstream, in that the code isn't being called anymore...)

[jackkoenig]

I pushed some changes, Mems weren't checking compile options properly.

In doing those changes I got an idea, what if requireIsChiselType also took compile options and didn't actually do the check for compatibility code? it would provide a more concise way for libraries to accept both chisel3 and Chisel code than what we're doing in Queue.

[jackkoenig]

So, if I recall correctly, we removed Vec.fill because it confused people when they were trying to create Chisel Types. Now that we're splitting Vec and VecInit, might it be time to bring it back?

[ducky64]

That sounds cleaner than VecInit(Seq.fill(...)).

[ducky64]

requireIsChiselType taking compile options: I really don't want to thread compile options through the code any more than is necessary, it just bloats the argument lists.

I also don't want library writers using compile options, and it shouldn't be part of the chisel external API (it's actually not exposed in chisel3._). (similarly with SourceInfo, at least until we develop abstractions that make sense for propagating library entry locators and internals locators). requireIsChiselType is also experimental, and I think we should look for better abstractions than having library writers haphazardly sprinkling require guards in their code.