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

Parse Floating Point Constants #2195

Closed
wants to merge 3 commits into from
Closed

Parse Floating Point Constants #2195

wants to merge 3 commits into from

Conversation

jiahanxie353
Copy link
Collaborator

This PR tries to address #2061
Support:

  1. parsing of floating point in the frontend and
  2. the conversion between "human-readable" decimal representation of floating point numbers and IEEE754 representation for json/data files.

jiahanxie353
jiahanxie353 commented Jul 3, 2024
View reviewed changes
tests/correctness/float/float-const.expect
},
"mem_write": {
"data": [
4.2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh the second thought is that it only proves that it reads/writes the same value, but doesn't prove that it's not necessarily intepretting 4.2 as the decimal representation..

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what you mean by that? The comment is lacking enough context to make sense to me.

rachitnigam
rachitnigam reviewed Jul 10, 2024
View reviewed changes
calyx-frontend/src/syntax.pest Outdated
@@ -26,7 +27,8 @@ num_lit = ${
~ ( "d" ~ decimal
| "b" ~ binary
| "x" ~ hex
| "o" ~ octal )
| "o" ~ octal
| "f" ~ float)
Copy link
Contributor

Choose a reason for hiding this comment

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

One challenging thing about this is that we are hardcoding IEEE-754 encoding for floating point and might eventually want to support things like posits (something that @Mark1626 was working on) so we might want a different way to specify floating point numbers?

@sampsyo thoughts? One thing I was thinking is that the syntax is only available in a magical primitive:

cf1 = ieee754_const(5.11231); // Error if exact representation is not possible

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiahanxie353 What do you think about this approach? Instead of allowing floating point constants, we define a primitive ieee754_const and only allow the parser to parse floating point values within as its parameters. In all other cases, we throw an error saying that floating point constants must be generated using this special cell.

This will require some trickery in the parser/well-formedness checker but hopefully not too complicated?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiahanxie353 ping about this!

@jiahanxie353 jiahanxie353 mentioned this pull request Jul 24, 2024
Open
@jiahanxie353
Copy link
Collaborator Author

Currently, it keeps saying:

Error: tests/correctness/float/float-const.futil
13 |        reg0.in = ieee754_const(32'f1.0);
   |                               ^ Parse error
expected guard_eq, guard_neq, guard_leq, guard_geq, guard_lt, guard_gt, guard_or, or guard_and

Any advice on how to fix the parsing?

@rachitnigam
Copy link
Contributor

I'm not sure what is happening with the PR: the diff is too big for me to read the changes you've added.

@jiahanxie353
Copy link
Collaborator Author

I'm not sure what is happening with the PR: the diff is too big for me to read the changes you've added.

Now it should cleaner!

rachitnigam
rachitnigam reviewed Aug 12, 2024
View reviewed changes
tests/correctness/float/float-const.futil
wires {
group read {
mem_read.addr0 = 1'b0;
reg0.in = ieee754_const(1.00);
Copy link
Contributor

Choose a reason for hiding this comment

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

@sampsyo thoughts on this representation as opposed to the idea of only having a primitive called ieee754 and using its output port?

@rachitnigam
Copy link
Contributor

@jiahanxie353 your pattern for float is:

float =   @{ ASCII_DIGIT+ ~ "." ~ ASCII_DIGIT+ }

So how would the following thing match it:

32'f1.0

rachitnigam
rachitnigam reviewed Aug 12, 2024
View reviewed changes
calyx-frontend/src/syntax.pest
@@ -16,17 +16,22 @@ binary = @{ ASCII_HEX_DIGIT+ }
decimal = @{ ASCII_HEX_DIGIT+ }
octal = @{ ASCII_HEX_DIGIT+ }
hex = @{ ASCII_HEX_DIGIT+ }
float = @{ ASCII_DIGIT+ ~ "." ~ ASCII_DIGIT+ }

ieee754_const = { "ieee754_const(" ~ float ~ ")" }
Copy link
Contributor

Choose a reason for hiding this comment

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

separate out iee754_const( into "ieee754" ~ "("

@rachitnigam
Copy link
Contributor

Another note from a synchronous chat: IEEE754 requires information on the number of bits being used.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has not seen activity in 14 days and is being marked as stale. If you're continuing work on this, please reply and state how to get this PR in a mergeable state or what issues it is blocked on. If the PR is not ready for review, please mark it as a draft. The PR will be closed in 7 days if there is no further activity.

@jiahanxie353
Copy link
Collaborator Author

This is blocking #1928

@github-actions github-actions bot removed the S: Stale label Oct 13, 2024
@andrewb1999
Copy link
Collaborator

This may become important for AMC soon too. What's the limiting factor here? It would also be nice to be able to support at least f16, f32, and f64.

@jiahanxie353
Copy link
Collaborator Author

This may become important for AMC soon too. What's the limiting factor here? It would also be nice to be able to support at least f16, f32, and f64.

iirc @rachitnigam took it over after a synchronous meeting; but i can work on this if @rachitnigam is busy

@rachitnigam
Copy link
Contributor

Right, I ended up being pretty busy so didn't have time to check up on this.

@rachitnigam
Copy link
Contributor

@jiahanxie353 remind me what problem you were running into?

@jiahanxie353
Copy link
Collaborator Author

@jiahanxie353 remind me what problem you were running into?

I remember the main thing was about the format and the parsing:

  1. We need to agree on a format.
    • We discussed about potential format like ieee754,
      • but we also need explicit number-of-bits information;
      • and we need a verifier to check that the user input number literal is actually representable in that number of bits by doing some decimal to floating point mantissa, exponent conversion.
  2. We also need to support the correct parsing:
    • How to coordinate the syntax.pest file and parser.rs adhering to the format above

@rachitnigam rachitnigam added the S: Blocked Issue is blocked label Oct 25, 2024
@rachitnigam rachitnigam mentioned this pull request Oct 25, 2024
Merged
@rachitnigam
Copy link
Contributor

Closing in favor of #2316. @jiahanxie353 once that is merged, can you make similar changes on the CIRCT side and make the rest of the flow work?

rachitnigam added a commit that referenced this pull request Oct 26, 2024
@rachitnigam
Floating-point Constants (#2316)
b6ef6bc 
Subsumes #2195.
rachitnigam added a commit that referenced this pull request Oct 26, 2024
@rachitnigam
Floating-point Constants (#2316)
6871ca1 
Subsumes #2195.
@jiahanxie353
Copy link
Collaborator Author

Thanks @rachitnigam !

once that is merged, can you make similar changes on the CIRCT side and make the rest of the flow work?

Sure, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rachitnigam rachitnigam rachitnigam left review comments

Assignees
No one assigned
Labels
S: Blocked Issue is blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jiahanxie353 @rachitnigam @andrewb1999