the cloneType and chiselCloneType hot mess 🔥

[ducky64]

Addresses #419

Overview:

• cloneType is now marked (through comments only) as an internal API.
• chiselCloneType deprecated (and changed to cloneTypeFull internally, analogous to cloneTypeWidth).
• chiselTypeOf(data) introduced as the external API to get a chisel type from a hardware object
• Intended usage: cloning is an implementation detail, and chisel types and hardware objects both should act as immutable types, with operations like Input(...), Reg(...), etc returning a copy and leaving the original unchanged. Hence, the clone operations are all deprecated.
• Deletes what appears to be an unused Bundle companion object.
• Input(...), Output(...), Flipped(...) require the object to be unbound

Remaining issues:

• Several uses of chiselCloneType remain in utils and tests for compatibility reasons, chiselCloneType does not check that the input is hardware bound while chiselTypeOf(...) does check. This goes into strictness issues with More of the bindings refactor #635 and needs more discussion.
• It would be nice for cloneType to return type Data, and have cloneTypeFull / etc fix up the types, because this.type is a nasty hack (see Invalid use of this.type for clone methods #193) and to avoid the .asInstanceOf[this.type] boilerplate needed in every cloneType implementation. It seems rocket-chip still has things that call cloneType directly so this causes compile errors. Only a small handful, though, so maybe this is something we could do? Otherwise, Scala does not seem to try implicit conversions on the wrong return type, so PML isn't a solution here.
• cloneType is not deprecate-able because it is simultaneously an internal and external API. Ideally, if the usages in rocket-chip are few, those could be fixed (with either chiselCloneType or chisel3-semantics chiselTypeOf) and the function (possibly could be?) made protected. (note, this will protect things within our control like Data.cloneType, but not user defined functions that don't explicitly protect their cloneType functions, especially Bundle sub-types)
• Cases where the user needs to define cloneType is where it get hairy, because that's where the intended abstraction does break down and users have to think about object uniqueness. Prime examples: ComplexAssign and RecordSpec test cases. Bundle annotations (or more advanced reflection-based cloning) might address some (perhaps even most) cases. Record especially might be difficult, because the elements can be defined however. This will probably be the biggest issue, especially since Bundle constructors can be deceptively wrong (what if you put a chisel type argument directly as a Bundle element and don't redefine the Bundle clone so the elements get cloned?)

This should not be merged yet, this is just a starting point to think about these issues.

[edwardcwang]

Should note (at least for now) the necessity of overriding cloneType for user-defined types (e.g. subclasses of Bundle).

[ducky64]

Resolution:

• For internal APIs that need to be exposed to Chisel._ compatibility (like chiselCloneType), qualify them into some namespace to prevent people from just using them.
• No resolution for the cloneType/chiselCloneType mess when the nice "all chisel types are immutable and you don't need to worry about object reference" abstraction breaks, because we don't know how to fix it. Goal is to reduce the number of cases where the user needs to override cloneType by incremental whackamole.

[jackkoenig]

Alright I've merged master into this (@ducky64 if you could check my work that would be great because it was nontrivial). Master is now bumped on rocket-chip for the first time in months so we can more easily check this against big code bases!

[ducky64]

@jackkoenig changes look good.

[jackkoenig]

Okay so the only problem I see with this is pretty minor but I want to see if there's a better solution. Basically, this PR makes it to where if you are using chiselCloneType (be it in compatibility or regular chisel3 code) you get inferred existential types. This is just an annoying warning, but I don't understand why it now comes up but didn't before. It's also problematic if you default to erroring on warnings (you know, good practice).

An example that illustrates the problem:

import chisel3._

class MyTestSubModule extends Module {
  val io = IO(new Bundle {
    val in = Input(UInt(32.W))
    val out = Output(UInt(32.W))
  })
  io.out := io.in
}

class MyTestModule extends Module {
  val child = Module(new MyTestSubModule)
  val io = IO(child.io.chiselCloneType)
  io <> child.io
}

object Test extends App {
  chisel3.Driver.execute(args, () => new MyTestModule)
}

This simply passes through the IO of a child module. On master, there is no warning, on this branch we get the existential type warning for the io of the parent module (MyTestModule).

Weirdly, if we change the io of the parent module to flip the child io type twice, the warning goes away:

class MyTestModule extends Module {
  val child = Module(new MyTestSubModule)
  val io = IO(Flipped(Flipped(child.io.chiselCloneType)))
  io <> child.io
}

Is there any way we can remove this warning without forcing users to change their code or build configuration?

[ducky64]

Strange... chiselCloneType was moved to compatibility using PML, so instead of returning this.type, it's now:

implicit class cloneTypeable[T<:Data](val target: T) extends AnyVal {
   def chiselCloneType: target.type = {
     DataMirror.internal.chiselTypeClone(target).asInstanceOf[target.type]
   }
 }

Also interestingly, IO is defined as

protected def IO[T<:Data](iodef: T): iodef.type = {

while Flipped is defined as

def apply[T<:Data](source: T): T = {

Does redefining IO to return T (instead of iodef.type) help? Or does that also break things? I'm not sure why IO is defined that way...

[jackkoenig]

Changing IO to return T instead of iodef.type helps for this specific case, but not for a minorly modified version:

class MyTestModule extends Module {
  val child = Module(new MyTestSubModule)
  val io = IO(new Bundle {
    val port = child.io.chiselCloneType
  })
  io.port <> child.io
}

However, changing chiselCloneType to return T instead of target.type did fix it. The question is why do they return the structural type instead of just the type parameter type?

[ducky64]

I'm not sure why IO returns a structural type, but if it works with parameter types, we should change it to that.
I think the reason chiselCloneType returns a structural type is because the old style (this.cloneType) returns this.type, and this was the minimal change. But again, I think the type parameter type should be preferred if it works. A long time ago, the uses of this.type was flagged as an issue which we're slowly solving.

[jackkoenig]

@ducky64 Can you change def IO back to returning iodef.type otherwise I think this is good for RC1