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

Implement some missing maths-related intrinsics #4095

Merged
merged 1 commit into from
Mar 1, 2016

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Feb 13, 2016

These intrinsics are used by the Rust compiler.

llvm_log_f32: 'Math_log',
llvm_log_f64: 'Math_log',
llvm_exp_f32: 'Math_exp',
llvm_exp_f64: 'Math_exp',
llvm_trunc_f64: 'Math_trunc',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there such things as llvm_trunc_f32 and llvm_floor_f32? If so, could add those as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@juj
Copy link
Collaborator

juj commented Feb 13, 2016

Is Math.trunc an asm.js import? It is not listed here at least: http://asmjs.org/spec/latest/#standard-library

@tomaka
Copy link
Contributor Author

tomaka commented Feb 13, 2016

@juj Oh I didn't realize these functions were part of an asm.js standard library. I just looked at a reference for the standard javascript Math object.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 13, 2016

It also looks like trunc is in fact not standard Javascript but an extension (or maybe it's ECMAScript 6, I'm not sure).
However it's quite simple to implement manually. Should that be done on the emscripten side or on the Rust side?

@kripken
Copy link
Member

kripken commented Feb 13, 2016

Emscripten side makes sense for things that are not asm.js builtins (then other projects can use them too). See e.g. llvm_uadd_with_overflow_i64 for an example.

For things that are asm.js builtins, adding them to library.js makes them work, but to make them fast we'd need them in the asm.js backend in LLVM. (But that can be done later, if needed.)

@juj
Copy link
Collaborator

juj commented Feb 13, 2016

I don't think we should reuse anything from Math if it means an FFI call, but have all of those in asm.js side. @kripken, @lukewagner: do you know if this list http://asmjs.org/spec/latest/#standard-library is out of date? It does not list Math.clz32 for example.

@kripken
Copy link
Member

kripken commented Feb 16, 2016

Yes, that does look out of date. @lukewagner, do you know what the correct list is?

@lukewagner
Copy link
Contributor

Looks like Math.trunc is new with ES6 so it's not in the asm.js stdlib. There are several trunc operators in wasm. I'd recommend inlining trunc in Emscripten for now.

@@ -1764,6 +1764,7 @@ var Math_imul = Math.imul;
var Math_fround = Math.fround;
var Math_min = Math.min;
var Math_clz32 = Math.clz32;
Copy link
Member

Choose a reason for hiding this comment

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

since trunc is fairly new in ES6, we should have a pollyfill.

@kripken
Copy link
Member

kripken commented Feb 16, 2016

Ok, given the optimization discussion, this looks good to go aside from needing a trunc polyfill, and also let's add some testing. Can add to test_llvm_intrinsics in tests/test_core.py, or an LLVM IR test in tests/cases would be ok as well.

@kripken
Copy link
Member

kripken commented Feb 20, 2016

What's the status here? Anything I can do to help?

@tomaka
Copy link
Contributor Author

tomaka commented Feb 20, 2016

Sorry, I was taken by the release of Vulkan. It should be good now.

@kripken kripken merged commit 868ee73 into emscripten-core:incoming Mar 1, 2016
@kripken
Copy link
Member

kripken commented Mar 1, 2016

Thanks, merged!

This uncovered a backend bug with powi, where asm.js wants their types to be the same. Fixed in the fastcomp repo.

@tomaka tomaka deleted the f64-maths branch March 1, 2016 19:23
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.

4 participants