Skip to content

Conversation

@pmwhite
Copy link
Contributor

@pmwhite pmwhite commented Jul 7, 2021

fix #897

This isn't finished by any means. What's missing is

  1. An implementation for fma
  2. Tests

My plan for 1 is to port the native ocaml runtime's C implementation to javascript. However, I'm not sure how to do 2, since the stdlib for ocaml 4.12 does not provide functions like Float.atanh or Float.acosh, so I can't even access the primitives in a straightforward way.

If I were to write tests and run them locally using an opam switch with the 4.13 compiler, it would still break the current CI. This seems like blocker for merging this PR, since it presumably will be impossible to test until 4.13 is released.

Am I understanding things correctly?

@hhugo
Copy link
Member

hhugo commented Jul 7, 2021

You can declare externals in your test directly. Just copy paste signature from ocaml master

@hhugo
Copy link
Member

hhugo commented Jul 7, 2021

I'd probably put new functions in the ieee_754.js file next to other related functions

@pmwhite
Copy link
Contributor Author

pmwhite commented Jul 8, 2021

Moved the functions to ieee_754.js. Looks like some of the existing functions in that file re-implement functions that are builtin. If this is to act as a polyfill, maybe we should just use the polyfill as a fallback. For example:

var caml_sinh_float = Math.sinh || function (x) { return (Math.exp(x) - Math.exp(-x)) / 2; }

@pmwhite
Copy link
Contributor Author

pmwhite commented Jul 8, 2021

Looks like compiler/test-ocaml/lib-float/test.ml is taken directly from the ocaml compiler. Oddly enough, it seems to be lacking tests for several of the builtin functions. It would be nice if the ocaml's stdlib tests were available in opam so that we could just depend on that instead of manually keeping these files up-to-date.

Anyway, I take it that tests should go in compiler/tests-jsoo/test_floats.ml

return function cbrt(x) {
return x < 0 ? -pow(x, 1/3) : pow(x, 1/3);
};
})(Math.pow);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why pass pow in like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these polyfill implementations are taken from the MDN docs. This one, in particular was taken from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/cbrt

@pmwhite
Copy link
Contributor Author

pmwhite commented Jul 8, 2021

Added some tests. The situation is not ideal because the ocaml stdlib doesn't have these functions defined yet, so these tests can't be run in native mode (I disabled native mode for all the tests in this directory, but if there's no workaround, I should probably put the float tests in their own library so we can disable native code for just the float tests.

@hhugo
Copy link
Member

hhugo commented Jul 9, 2021

Moved the functions to ieee_754.js. Looks like some of the existing functions in that file re-implement functions that are builtin. If this is to act as a polyfill, maybe we should just use the polyfill as a fallback. For example:

var caml_sinh_float = Math.sinh || function (x) { return (Math.exp(x) - Math.exp(-x)) / 2; }

We don't need polyfill for things that are well supported. It's possible that polyfill were added in the past if support was not great at the time.

@hhugo
Copy link
Member

hhugo commented Jul 9, 2021

Added some tests. The situation is not ideal because the ocaml stdlib doesn't have these functions defined yet, so these tests can't be run in native mode (I disabled native mode for all the tests in this directory, but if there's no workaround, I should probably put the float tests in their own library so we can disable native code for just the float tests.

Maybe move the tests in theirs on lib for now.

@pmwhite
Copy link
Contributor Author

pmwhite commented Jul 22, 2021

@hhugo I'm finished implementing and testing these functions, so this is ready to be reviewed. The fma implementation I took from this gist, and the fma tests I took from the ocaml compiler. The aforementioned gist passed all the tests without modification.

@hhugo hhugo changed the title WIP: Implement float primitives for 4.13 Implement float primitives for 4.13 Jul 28, 2021
@hhugo
Copy link
Member

hhugo commented Jul 28, 2021

I've tested your PR with 4.13 and made a small fix regarding nan printing in the tests. I've also rebased your work on top of the latest master.

@hhugo
Copy link
Member

hhugo commented Jul 28, 2021

Would you mind adding a changelog entry ?

@hhugo hhugo merged commit 23b25ca into ocsigen:master Jul 29, 2021
@pmwhite
Copy link
Contributor Author

pmwhite commented Jul 29, 2021

Ahh, sorry for being slow to correct the formatting and changelog. Thanks!

@hhugo
Copy link
Member

hhugo commented Jul 29, 2021

Thanks for working on this

kit-ty-kate pushed a commit to kit-ty-kate/opam-repository that referenced this pull request Sep 5, 2021
…s_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-ocamlbuild, js_of_ocaml-lwt and js_of_ocaml-compiler (3.10.0)

CHANGES:

## Features/Changes
* Compiler: add support for OCaml 4.13
* Compiler: new tool to check for missing primitives
* Compiler: drop support for OCaml 4.03 and bellow
* Lib: add offsetX and offsetY to Dom_html.mouseEvent
* Lib: add innerText property for Dom_html
* Runtime: add dummy implementation for many dummy primitives
* Runtime: add runtime for new float operation in 4.13 ocsigen/js_of_ocaml#1113 (by pmwhite)

## Misc
* manual/rev_bindings.wiki: fix compilation error
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.

Runtime: implement new runtime for floats operation ..

3 participants