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

Brilift: Float support #214

Merged
merged 6 commits into from
Jun 21, 2022
Merged

Brilift: Float support #214

merged 6 commits into from
Jun 21, 2022

Conversation

sampsyo
Copy link
Owner

@sampsyo sampsyo commented Jun 20, 2022

Not much to see here; this just adds support for the ever-popular floating point language extension in the Brilift compiler.

As always, there is a dangling annoyance here that different tools print floats differently. Currently, printf is currently only roughly within the ballpark of how Node and Rust print floats, so there are some niggling mismatches. A more tolerant way to match this stuff would be helpful, but I don't have any great ideas currently. Maybe some kind of fuzziness in Turnt, someday.

Some errors remain, and Turnt testing only works some of the time
because of floating point accuracy issues.
Some benchmarks contain stuff like `x: float = const 2` where I would
prefer `const 2.0`. This is impractical to rule out, probably, because
JavaScript (and hence JSON) does not distinguish between number types.
Just tolerate those integers!

Everything seems to work now, up to FP printing precision issues.
Apparently, a double always needs 17 decimal digits to print
unambiguously:
https://stackoverflow.com/a/10896466

So here we are. Solves most of the mismatches.
All this Infinity/-Infinity stuff should probably actually have a spec,
rather than just doing what Node does. But ah well. Someday.
@sampsyo
Copy link
Owner Author

sampsyo commented Jun 20, 2022

I think a good way to deal with the FP inconsistency stuff (in a separate PR) would be to just specify & require that all implementations use exactly 17 decimal digits of precision. We would bypass "friendly" printing modes that attempt to do aesthetically pleasing but still round-trippable rounding, as in Python >=3.1:

$ python3                                                                     ~
Python 3.9.13 (main, May 24 2022, 21:13:51) 
[Clang 13.1.6 (clang-1316.0.21.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> 9/-20
-0.45
>>> '{:.17f}'.format(9/-20)
'-0.45000000000000001'

@Pat-Lafon
Copy link
Contributor

Should other parts of floating point be specified as well, specifically how a compliant interpreter/runtime outputs Nan/Infinity? Are these only runtime values or should tools like bril2txt/bril2json be able to parse them as well?

@Pat-Lafon
Copy link
Contributor

I am also interested in the following:

If there should be 2 additional operators to the floating point extension, isnan and isinf, which would match similar behavior as seen in Python. Though I guess that this could be implemented as a small function calls in a Bril program and then used via the import extension in the niche cases that would want them.

If Bril should allow conversion between integers and floating point values with an operator like toint/tofloat.

@sampsyo
Copy link
Owner Author

sampsyo commented Jun 21, 2022

Yes, all that sounds right! Namely, specifying the output for NaN and infinity (doesn't matter what, but it doesn't have to match Node's defaults either) seems like a great idea. As does adding isnan and isinf, which match the behavior of math.h. This should be the plan. Conversion to/from integers seems pretty cool too.

Not sure about the text parser; that seems like a lower-order concern because the text format doesn't really have a spec. Agreement between implementations is nice anyway though, as you've experienced.

@Pat-Lafon
Copy link
Contributor

Not sure about the text parser; that seems like a lower-order concern because the text format doesn't really have a spec. Agreement between implementations is nice anyway though, as you've experienced.

What I actually meant with that question is the following. In the canonical Bril JSON IR which one would provided to a compliant interpreter, is/should there be a way to provide the following.

{
  "functions": [
    {
      "instrs": [
        {
          "dest": "v0",
          "op": "const",
          "type": "float",
          "value": "NaN"
        }
      ],
      "name": "main"
    }
  ]
}

Or are values like NaN strictly something that can occur at runtime.

@sampsyo
Copy link
Owner Author

sampsyo commented Jun 22, 2022

Got it; thanks for clarifying! To be clear, at the moment these are not allowed because NaN/infinite literals do not appear in JSON. I have no objection to extending the AST to allow these values in constants—no strong opinion either way, really.

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.

None yet

2 participants