Skip to content

Update internal Pipe wiring - fixes #615#616

Merged
ucbjrl merged 1 commit intomasterfrom
brokenpipe615
May 25, 2017
Merged

Update internal Pipe wiring - fixes #615#616
ucbjrl merged 1 commit intomasterfrom
brokenpipe615

Conversation

@ucbjrl
Copy link
Copy Markdown
Contributor

@ucbjrl ucbjrl commented May 24, 2017

Replace ambiguous bi-connect ("<>") with mono-connect (":=") for internal Pipe wiring.

Replace ambiguous bi-connect ("<>") with mono-connect (":=") for internal Pipe wiring.
@ucbjrl ucbjrl requested a review from ducky64 May 24, 2017 18:27
Copy link
Copy Markdown
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pipe fix looks good.

I'm a bit confused about the test though, what is it checking for? It's not using the Pipe apply code that was changed, and it appears to be checking for behavior that is no longer in the codebase?

@ucbjrl
Copy link
Copy Markdown
Contributor Author

ucbjrl commented May 24, 2017

It reproduces the original failure reported by SpaceCowboyMax. It will fail if the mono-connections are replaced with the original bulk/bi connections.

Copy link
Copy Markdown
Contributor

@ducky64 ducky64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. Looks fine.

Side note: strange that the companion object is what actually builds the hardware, and that the class itself calls the companion object, instead of the companion object being a constructor for the class.

@aswaterman
Copy link
Copy Markdown
Member

@ucbjrl We learned a few years ago that a common case of Pipe (register retiming) depends on the registers being in the same module as the unit being retimed, which is why the companion object instantiates them directly.

The Module named Pipe only exists for backwards compatibility, to support old code that writes Module(new Pipe) instead of Pipe. Maybe the Module should be moved into the compatibility layer.

@ducky64
Copy link
Copy Markdown
Contributor

ducky64 commented May 24, 2017

Agree, we should document the rationale as well as deprecate the class (if possible) / move it into compatibility. Might be a separate PR though.

@ucbjrl ucbjrl merged commit 0d121a2 into master May 25, 2017
ucbjrl added a commit that referenced this pull request May 25, 2017
Replace ambiguous bi-connect ("<>") with mono-connect (":=") for internal Pipe wiring.
(cherry picked from commit 0d121a2)

bump version
@ucbjrl ucbjrl deleted the brokenpipe615 branch May 25, 2017 18:56
ucbjrl added a commit that referenced this pull request May 26, 2017
Replace ambiguous bi-connect ("<>") with mono-connect (":=") for internal Pipe wiring.
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.

3 participants