Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code duplication/divergence in (System)Verilog backends is going to bite us someday. #220

Open
cbiffle opened this issue May 3, 2017 · 3 comments
Open

Code duplication/divergence in (System)Verilog backends is going to bite us someday. #220

cbiffle opened this issue May 3, 2017 · 3 comments
Labels
enhancement

Comments

@cbiffle
Copy link
Contributor

cbiffle commented May 3, 2017

The Verilog and SystemVerilog backends have a lot in common. Nearly parallel code. Nearly identical primitives.

Nearly. They diverge in places. Some of those places are probably intentional.

The two target languages are so similar that the backends ought to share code. I'm filing this to seek feedback before I go crazy and clean anything up.

I see two obvious options:

  1. Factor shared code into a separate library.
  2. Fold the two backends into a single library.

My preference would be for option 2 -- the rest of the compiler would not notice, and it reduces boilerplate.
cbiffle added a commit to cbiffle/clash-compiler that referenced this issue May 3, 2017
@cbiffle
Verilog: truncate Integer when converting to sized
7c096f0 
Integer often winds up being represented by 64 bits.  When using
fromInteger# to assign to, say, a 'BitVector 8', this is fine in
Verilog, because the top 64-8 bits get discarded.

But because the template is an expression template, it can also get
applied inside of a bitstring concatentation like

    {x, y, $unsigned(an_integer), z}

Here the difference between an 8-bit and a 64-bit value changes the
meaning of the expression.

This commit explicitly slices Integers when moving into four common
sized integer-like types.

See clash-lang#219 (this may not be a complete fix for that bug) and clash-lang#220 (filed
as a result of this work)
cbiffle added a commit to cbiffle/clash-compiler that referenced this issue May 3, 2017
@cbiffle
Verilog: remove stray paren from BitVector quot#
85012a9 
This primitive template was syntactically invalid, so I can only assume
we never generate it?

Found during my expression template review under clash-lang#219.

Note that the same mistake has appeared in both the Verilog and
SystemVerilog backends; see clash-lang#220.
@christiaanb
Copy link
Member

christiaanb commented May 3, 2017 •
edited
Loading

@thoughtpolice is planning to fold the current backends into clash-lib: I think as part of this, seeing what can be shared between the Verilog and SystemVerilog backends would be a good idea.

The reason they don't share code right now is mostly for pragmatic/lazy reasons:

  • Together with @Ericson2314 we wrote current backend code to get SystemVerilog generation aside from VHDL generation. We picked SV because SV has more in common with VHDL than Verilog in terms of supporting things like structs (and arrays at module boundaries)
  • I then started a Verilog backend, and instead of making sure I could share code between SV and Verilog backend, I was lazy, and just copied code...

Anyhow, option 2 will (eventually) happen when the current backends get merged into clash-lib

@cbiffle
Copy link
Contributor Author

cbiffle commented May 4, 2017

Thanks for the historical context. I'm happy to hold off and let @thoughtpolice do this, or to pitch in myself.

@thoughtpolice
Copy link
Contributor

I need to revive my WIP branch. Just moving everything in isn't too hard; I'll get to it soon...

@christiaanb christiaanb added this to the 1.1 milestone Dec 18, 2018
@christiaanb christiaanb modified the milestones: 1.1, 1.2 Jan 16, 2020
@martijnbastiaan martijnbastiaan removed this from the 1.4 milestone Jan 19, 2022
leonschoorl pushed a commit that referenced this issue Jul 31, 2023
@lmbollen
Add scatterUnitWb and gatherUnitWb instances (#220)
a0cbc40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@thoughtpolice @cbiffle @christiaanb @martijnbastiaan