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.

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

Calyx wrapper for Berkeley HardFloat Verilog library #1928

Open
wants to merge 31 commits into
base: calyx-float
Choose a base branch
from

Conversation

jiahanxie353
Copy link
Collaborator

Implement a wrapper in Calyx for Berkeley HardFloat fNToRecFN (IEEE standard format to HardFloat recoded format) Verilog module (adopted based on PyMTL's corresponding file).

And include a corresponding test case to convert from standard format to recoded format when the input floating-point number is a normal number.

I want to first create a draft PR to make sure that I'm on the right track. And I will include more test cases, such as subnormals, NaNs; and work on other HardFloat modules.

@rachitnigam
Copy link
Contributor

Woo! Awesome! This is exactly what I had in mind as well! Couple of things:

  • To get the CI working, you need to open a branch in the repo. I've invited you as a collaborator the repo so you should be able to do this
  • We should talk about where this code is going to live exactly. I was imagining keeping this in a separate repo but that would make testing more difficult. We can keep this in the repo under primitives/float?
  • We should also extend the Calyx data format to allow users to write down exact floating point values instead of having to work with unsigned representations. This magic happens in json_to_dat.py which uses the numeric_types.py code.

@jiahanxie353
Copy link
Collaborator Author

  • To get the CI working, you need to open a branch in the repo. I've invited you as a collaborator the repo so you should be able to do this

Just opened up calyx-float! Though it shows "This branch has not been deployed"?

  • We should talk about where this code is going to live exactly. I was imagining keeping this in a separate repo but that would make testing more difficult. We can keep this in the repo under primitives/float?

Agree. Yes primitives/float sounds reasonable.

  • We should also extend the Calyx data format to allow users to write down exact floating point values instead of having to work with unsigned representations. This magic happens in json_to_dat.py which uses the numeric_types.py code.

Sounds good! So we start with first extending the existing parser?

@jiahanxie353
Copy link
Collaborator Author

Hi @rachitnigam ! Just want to share an update that the current wrapper can perform floating add/sub and multiplication!

So I think I can move on to extending the user input data type to conduct more thorough tests. Any thoughts or tips on initiating and streamlining this process? Thanks!

@jiahanxie353 jiahanxie353 marked this pull request as ready for review February 21, 2024 21:19
@rachitnigam
Copy link
Contributor

User inputs seem like the right next step. Do you want to create a plan to scope out the work in this PR? For example one primitive plus fud extension should be enough

@jiahanxie353
Copy link
Collaborator Author

Do you want to create a plan to scope out the work in this PR? For example one primitive plus fud extension should be enough

Sure!

What does "fud extension" mean here, fud numeric type extension for floating-point values?

And do you agree that Calyx users will only use normal floating-point values, and at most, they will use IEEE format representation, and that they barely want to interact with HardFloat specific formats like "recoded" formats and "raw deconstructions"?

If that's the case, then I think ideally, I'd start with primitive addFN, which does two standard IEEE format floating-point numbers addition (and it doesn't exist yet).

Now I only have primitive fNToRecFN, recFNToFN, and primitive addRecFN to wrap their original HardFloat Verilog modules. So the current addition is executed as a workaround:

  1. taking two standard IEEE format floating-point numbers, convert them to HardFloat recoded format;
  2. use recoded format addition addRecFN to add two numbers;
  3. convert the result back to standard form using recFNToFN.

If we indeed want an addFN Calyx primitive, I can start with first making a separate Verilog file/module called addFN and chain up the above three steps. And finally declare a Calyx primtive to wrap this module by using extern. Then I can extend it in Calyx.

@jiahanxie353
Copy link
Collaborator Author

Do you want to create a plan to scope out the work in this PR?

Hi @rachitnigam , I think I have some idea about how to proceed! Can you check my plan to see if it makes sense?

  1. Create a new class called FloatPoint in numeric_types.py and add the corresponding json_to_data transformation.
  2. Modify the addFN test case to use floatpoint as the numeric_type and check the correctness.

@rachitnigam
Copy link
Contributor

Hi @jiahanxie353, I'm traveling a lot this month so I'll recommend that you go ahead and try things out instead of waiting on my responses. Folks in the Calyx zulip and slack should also be able to help.

@jiahanxie353
Copy link
Collaborator Author

Sure thing, sounds good!

@jiahanxie353
Copy link
Collaborator Author

Hi @rachitnigam , just finished a milestone that we can support passing floating point values in json and perform two floating-point numbers addition when using fud!

@@ -0,0 +1,16 @@
extern "addFN.sv" {
primitive addFN[
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this implemented combinationally? If so, we should mark it as a comb primitive instead of primitive. Also, we should probably not implement this as a combinational primitive at all since that is unlikely to meet timing?

@rachitnigam
Copy link
Contributor

Looks cool! Couple of notes:

  1. Seems like all the test inputs are hard-coded. We probably instead want to specifying these as floating point numbers in the fud .data files.
  2. The adder is implemented combinationally which will probably not meet timing on real designs. We should make it take some number of cycles. @andrewb1999 any thoughts?

@andrewb1999
Copy link
Collaborator

Yeah we definitely want floating point units to be pipelined when used in real designs. I think berkeley hardfloat is all combinational? If that's the case we can maybe add registers at the beginning and/or end and hope that retiming will do a decent job. In Vitis HLS all the floating point units I've seen are between 3 and 5 stage pipelines. I think this is definitely another case where allowing the latency to be parameterized would be very helpful.

@rachitnigam
Copy link
Contributor

@jiahanxie353 I think the move is to pipeline the output value of the adder by 4 cycles for now. Once we have a set of primitives and some designs, we can push them through the FPGA flow and see if designs are correctly being mapped onto DSPs on the FPGA (which are hardened blocks for efficiently performing operations likes add and mult).

Once this change is done, I think you're good to merge! Next step would be to change fud to ingest and output numbers in floating-point representation when the data-format is set to "float" instead of "bitnum".

@sampsyo
Copy link
Contributor

sampsyo commented Feb 27, 2024

Excellent. I'm late to the party, but I agree with where everything has ended up: namely, @jiahanxie353, your plan enumerated above sounds right to me, and @rachitnigam's suggested next steps (test inputs from data files, and a fixed 4-cycle latency) are also the right things to do here.

To expand in a possibly-unnecessary way about the 4-cycle latency: basically, the idea in Berkeley HardFloat is that all the operations are provided as combinational logic, but realistic designs do not want combinational FP ops. So its intended use case is that people just stick "useless" registers in front of or behind the HardFloat operation, and the EDA toolchain does its best to perform retiming (magically transforming the combinational-plus-registers description into a proper, balanced pipeline). So one option would be to expose these as comb primitives and call it good for now, but we would eventually want to add sequential versions that would be more practical. Another option is to go ahead and pick a latency (4 is fine!); then we'll be slightly closer to having a practical library from the start, in addition to a functionally correct one.

@sampsyo
Copy link
Contributor

sampsyo commented Feb 27, 2024

One extra category of logistical discussion:

  • Do we need to think about licensing here? Looks like HardFloat uses an MIT (ish?) license, which means we need to preserve the copyright notice and attribution somewhere.
  • Will it be a pain to check in a snapshot of the HardFloat Verilog code? This could make it semi-annoying to adapt to upstream changes. An alternative would be to provide a script to download the most recent version instead.

@jiahanxie353
Copy link
Collaborator Author

jiahanxie353 commented Feb 28, 2024

Thanks for all the advice folks! Got a 4-cycle adder by using dummy registers.

As per

One extra category of logistical discussion:

  • Do we need to think about licensing here? Looks like HardFloat uses an MIT (ish?) license, which means we need to preserve the copyright notice and attribution somewhere.

I believe you are right. That's why I kept

This Verilog include file is part of the Berkeley HardFloat IEEE Floating-Point Arithmetic Package, Release 1, by John R. Hauser.
Copyright 2019 The Regents of the University of California. All rights reserved.

in the source files like here for attribution. Is there anything else I need to do for copyright?

  • Will it be a pain to check in a snapshot of the HardFloat Verilog code? This could make it semi-annoying to adapt to upstream changes. An alternative would be to provide a script to download the most recent version instead.

It'll probably be annoying to adapt to upstream changes in the future if we just use a static snapshot. The issue is that HardFloat is implemented in Chisel in their repo. Implementing a translator/transpiler from Chisel to Verilog could (?) solve the concern but I doubt it's worth the effort. And since there's no direct Verilog implementation, I believe people just use the Verilog files obtained from a zip file in this page.

@rachitnigam
Copy link
Contributor

This all sounds good!

One extremely tangential note: "transpiler" is not a meaningful term: https://rachit.pl/post/transpiler/

Chisel already has a compiler to Verilog.

@sampsyo
Copy link
Contributor

sampsyo commented Feb 29, 2024

Got it; thanks, @jiahanxie353! Here's my suggestion about how to deal with including external source code like this. I think we have two options:

  • Check the HardFloat Verilog released code into our repo (this is called "vendoring"). With this route, it would be best to preserve the structure of the released code as identically as possible: that is, maybe we literally have a subdirectory inside primitives/float called HardFloat-1 (the name of the zip archive), and within that we preserve COPYING.txt, README.txt, and the source subdirectory. We could decide to exclude some files, but we would not modify any files—they would be preserved byte-for-byte as in the zip folder, not renamed/relocated or anything. This has the advantage of (a) making the licensing issues completely clear, as we are preserving literally the license from the original distribution, and (b) making it extremely clear what one would have to do if HardFloat were ever to be updated (just expand the zip into the right location; no further changes necessary).
  • Fetch the zip on demand. That is, we check in a little get_hardfloat.sh script that essentially does curl -LO http://www.jhauser.us/arithmetic/HardFloat-1.zip followed by an unzip. People would have to run this before they can use the float library. While this is obviously annoying to require, the advantages are that (a) it is even clearer what is our code and what is external code, (b) grepping in our repo will not be contaminated by matches in unrelated Verilog, and (c) it will be literally impossible for anyone to modify the HardFloat sources in the future, making them diverge from upstream.

Anyway, I think either could work! Just wanted to lay out the options.

@jiahanxie353
Copy link
Collaborator Author

Got it, I'll take the second fetch-and-unzip approach!

While working on it, got a decision to make regarding (1) changing the HardFloat source code after fetching it; (2) modifying verilator and icarus in fud.

  1. Option 1 changing the HardFloat source code is inspired by pymtl3-hardfloat, where it adds an includeFile.v and add an extra include line in the common included file in the source files, recFNToFN, so that every file in the source code can now cross reference.
    This is straightforward but since this changes the source file (though only after unzipping), will this mess up with licensing issues?

  2. The second approach involves changing fud stages. Take compile through icarus as the example, we need to change cmd and compile_with_iverilog.
    This might be more programmable but I'm not sure if it's worth it change it just for the sake of float? And I know we are undergoing changes to fud2.

Do you have any preference over 2 options?

@sampsyo
Copy link
Contributor

sampsyo commented Mar 2, 2024

I see! Good question… do you think you could elaborate a tiny bit more on the problem that each of these changes would solve? Here is my guess, but I am low-confidence:

The way (most? all?) Calyx primitives currently work is that the Verilog code for each primitive gets inlined into the generated Verilog code. The result is an entirely self-contained Verilog program, consisting of both primitive code and Calyx-compiled code, that we can compile all by itself. HardFloat presents another level of logistical complexity because, of course, we need to include a bunch of external files. The current solution is that our addFN.sv primitive implementation includes the relevant HardFloat source files:

`include "primitives/float/source/fNToRecFN.v"
`include "primitives/float/source/addRecFN.v"
`include "primitives/float/source/recFNToFN.v"

This in turn requires the actual HardFloat source files to use the same path prefix to import one another:

`include "primitives/float/source/HardFloat_primitives.v"

Obviously, the "pristine" HardFloat source files don't know about the path primitives/float, so that line isn't present as written there. In the actual fNToRecFN.sv source file as released, it's just:

`include "HardFloat_localFuncs.vi"

…and that doesn't necessary work by default to include the file relative to the containing source file, for some reason. I don't understand Verilog!!! It seems like it should just work! It is annoying that it doesn't!!!!!!!

  • Actually modify the HardFloat source files to hack them to use our blessed import paths.
  • Require all consumers of the generated Verilog (not just Icarus and Verilator—anyone who uses the generated Verilog to do anything, such as other EDA tools!) to support file-relative includes. For example, Verilator seems to have a --relative-includes option that enables this kind of include. And Icarus seems to have -grelative-include that does the same thing. So I believe @jiahanxie353's proposal is to add this flag to those simulators? But presumably every other user of the generated Verilog would need the same kind of option?

Is that a correct summary of the situation? If so, just adding the flag seems way easier to me… clearly, there is more we should do around the problem of Calyx now not emitting self-contained Verilog (it now has includes that depend on other files). But just adding the flag seems like a simple way to deal with it… if we think that other Verilog-consuming tools (Vivado? OpenROAD?) can also support this kind of include. Do you think that's true?

@jiahanxie353
Copy link
Collaborator Author

Yes, that's pretty much the situation and grelative-include can solve most of them. And please let me add a few points that makes this tricky.

Firstly, the "pristine" HardFloat has this wacky kind of import:
https://github.com/pymtl/pymtl3-hardfloat/blob/aea69e416e079178df4bb5697795ef653f55df58/HardFloat/source/mulRecFN.v#L39-L40

Even if they intended to mean relative import, they should at least write:

`include "RISCV/HardFloat_specialize.vi"

But anyhow, this is not difficult to solve. And my proposed solution is to append -I calyx/primitives/float/HardFloat-1/source/RISCV to icarus.py file when compiling to Verilog, which can be done for example, add an include_path argument to

def compile_with_iverilog(

A trickier thing seems unsolvable by using grelative-include and -I flag. Take the example you mentioned:

`include "primitives/float/source/HardFloat_primitives.v"

This line is manually inserted by me, and there's no such import in the "pristine" HardFloat.

And in fact, there's no single file that has "include "HardFloat_primitives.vi" despite the fact HardFloat_primitives.vi contains essential helper modules like countLeadingZeros, which is being used throughout, such as in fNToRecFN.v and addRecFN.v (so this means these files are just using modules that are from nowhere!)

As a workaround, pymtl manually created this includeFile.v, which includes HardFloat_primitives.v. And then they insert it, by changing the source code, to recFNToFN.v because they found out that this is a commonly shared file across the source code.

@rachitnigam
Copy link
Contributor

I haven't kept track of this thread that well but one thing that would be good to have is being able to have a default flow that generates exactly one Verilog file like we do today. We can emit some information information the user that they've requested that we link with HardFloat and therefore are implicitly agreeing to its LICENSE. We also should be carefully when we provide a new release on crates.io if we place Hardfloat in calyx-stdlib.

jiahanxie353 and others added 24 commits October 28, 2024 19:00
…nclude and library files include for iverilog
rachitnigam added a commit that referenced this pull request Nov 1, 2024
Extract floating-point support for `fud` from
#1928
@rachitnigam
Copy link
Contributor

@jiahanxie353 with the new FP changes, seems like we'd want to merge this relatively soon. I think we should split this into two PRs: one that adds support for morty to manage multi-file primitives and another actually adding the hardfloat stuff to the repo. I think we should design the morty stuff well so that we can actually use it for other things in the future.

@jiahanxie353
Copy link
Collaborator Author

@jiahanxie353 with the new FP changes, seems like we'd want to merge this relatively soon. I think we should split this into two PRs: one that adds support for morty to manage multi-file primitives and another actually adding the hardfloat stuff to the repo. I think we should design the morty stuff well so that we can actually use it for other things in the future.

Sounds good, I'll do that!

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.

5 participants