Skip to content

Conversation

@albert-magyar
Copy link
Contributor

Two changes:

IrrevocableIO is now a subtype of DecoupledIO, which was already assumed and implied by the documentation of the up/downconversions between them.

EnqIO (the object used to create the output of Queue) now returns an instance of type IrrevocableIO, which reflects a guarantee provided by queues. This seems more-or-less to be central to the "queue-ness" of a module.

Understandably, this creates a new burden that must be maintained in future implementations to avoid breaking the API. This enables users of AdvTester to use queues properly on the outputs of their designs.

@hcook
Copy link
Member

hcook commented Jan 11, 2017

I'm pretty suspicious of the subtyping. We definitely wrote it that way the first time and then changed it because we came up with a case that was problematic. I think it had something to do with the directionality of the interface. If you have a function you're going to map over the inputs to something that expects Irrevocable behavior, you don't want that to be able to accept DecoupledIOs...

As far as the output of queue goes, we then left it alone for backwards compatibility reasons.

@albert-magyar
Copy link
Contributor Author

Not sure what the problem would be in that function example, since (IrrevocableIO) => ... won't accept DecoupledIO in either case?

It also looks like EnqIO and DeqIO are backwards, but that's a somewhat separate issue.

@aswaterman
Copy link
Member

@hcook's right; the discussion leading to that conclusion is here: #274

@albert-magyar
Copy link
Contributor Author

albert-magyar commented Jan 12, 2017

It looks like the issue @sdtwigg raised is with (DecoupledIO) => ... accepting IrrevocableIO, not the other way around.

I understand that having the constrained IrrevocableIO subtype the less constrained DecoupledIO doesn't feel very Liskov substitution-y, but at least the runtime check will catch violations in simulation. The issue with attempting to push it more to the type system by eliminating IrrevocableIO <: DecoupledIO is that there are still huge holes.

Since IrrevocableIO <: ReadyValidIO, the example Stephen posited is still possible (substituting ReadyValidIO for DecoupledIO) under the current type hierarchy. The hole exists in some form as long as IrrevocableIO subtypes any subtype of Bundle rather than using composition and a has-a relationship.

Much bigger issue: even without the subtyping, the following is perfectly legal (compile and elaboration) under current Chisel3.

def ConnectWrong[T <: Data] (source: DecoupledIO[T], sink: IrrevocableIO[T]) = {
  source <> sink
}

@azidar azidar self-requested a review February 1, 2017 20:31
@ducky64
Copy link
Contributor

ducky64 commented Feb 22, 2017

We're keeping the same type hierarchy and fixing the advanced tester to issue warnings. Re-open if you disagree...

@ducky64 ducky64 closed this Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants