rust-lang / libm

A port of MUSL's libm to Rust.
Apache License 2.0
536 stars 96 forks source link

Goals and usecases for libm #195

Closed gnzlbg closed 5 years ago

gnzlbg commented 5 years ago

There are a bunch of changes being proposed and merged for libm that deal with the "architecture" and I have the feeling that everyone has a different idea of the goals of the project.

I think it would be worth it to collect all possible goals and use cases, before we start discussing which ones of them do we want to pursue, and which ones would be out-of-scope. I've written a small document that collects the goals that came to my mind. It can be collaboratively edited, so please do add everything that comes to your mind.

Once that is done, I'll summarize it, and we can start having a meaningful discussion about which goals of that list should the project have. Once we have consensus on that, it will be easier to know on a case-by-case basis whether the changes being proposed are progress towards those goals or not, and which architecture we should have for achieving those goals.

(If necessary we could have a small 15 min meeting about the goals after the list is collected and the summary is written with stakeholders)

cc @alexcrichton @Schultzer @burrbull @m1el @rrbutani @rust-lang-nursery/libm

alexcrichton commented 5 years ago

As of a few months ago this was basically a dead crate and/or on life support and we needed changes for wasm. I've tried to keep it alive since then and put it on a solid foundation again by getting testing up and running. I am not personally available to lead the charge on large refactorings, major new features, etc.

That being said this is an extremely important crate now to the standard library and rust-lang/rust due to its default usage on basically all targets for some math intrinsics. This isn't a crate that should be taken lightly and pulled in whichever direction is necessary.

I don't think there's really an option right now to add as many features as possible here, but rather this crate needs to remain focused on being a streamlined implementation for various math functions that are commonly found in libm C libraries.

gnzlbg commented 5 years ago

That being said this is an extremely important crate now to the standard library and rust-lang/rust due to its default usage on basically all targets for some math intrinsics.

Can you elaborate on how the standard library uses this crate ? I don't find any references to it in rust-lang/rust .

EDIT: it's used in compiler-builtins: https://github.com/rust-lang-nursery/compiler-builtins

jacobrosenthal commented 5 years ago

Related is if libm should be usable across the no_std ffi barrier. Quite often you want to ffi some existing c code which needs libm, and libm needs libc, but libc is generally conflicting so you dont want to use either if possible. Its possible to manually copy paste and extern no mangle the libm function I need, but this isn't particularly maintainable. https://github.com/jacobrosenthal/cmsis-dsp-sys/blob/master/src/lib.rs#L24-L25

It would be ideal to somehow expose those functions as a crate using a feature flag maybe?

another not ideal solution is maybe separately building c static lib and linking that in 2 steps, maybe related to https://github.com/rust-lang-nursery/libm/pull/192 but boy would be nice to have a workflow for that.. especially in a non xargo world this seems painful.

or maybe performance of this crate will never be no_std optimized and maybe I should go poke at https://github.com/NeoBirth/micromath ?

Thoughts? Thanks

On Wed, Jul 3, 2019 at 12:53 AM gnzlbg notifications@github.com wrote:

There are a bunch of changes being proposed and merged for libm that deal with the "architecture" and I have the feeling that everyone has a different idea of the goals of the project.

I think it would be worth it to collect all possible goals and use cases, before we start discussing which ones of them do we want to pursue, and which ones would be out-of-scope. I've written a small document that collects the goals that came to my mind:

https://paper.dropbox.com/doc/Goals-for-Rust-libm--AgOlAUNFtFswIE9Y8_BIhzgsAg-gRlnRke09h8fNcIeMGk4v

It can be collaboratively edited, so please do add everything that comes to your mind. Once that is done, I'll summarize it, and we can start having a meaningful discussion about which goals of that list should the project have. Once we have consensus on that, it will be easier to know on a case-by-case basis whether the changes being proposed are progress towards those goals or not, and which architecture we should have for achieving those goals.

(If necessary we could have a small 15 min meeting about the goals after the list is collected and the summary is written with stakeholders)

cc @alexcrichton https://github.com/alexcrichton @Schultzer https://github.com/Schultzer @burrbull https://github.com/burrbull @m1el https://github.com/m1el @rrbutani https://github.com/rrbutani @rust-lang-nursery/libm

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang-nursery/libm/issues/195?email_source=notifications&email_token=AADPI5BZFVUWSLRGD3J26BDP5RLJHA5CNFSM4H5C6QUKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G5B7LSA, or mute the thread https://github.com/notifications/unsubscribe-auth/AADPI5HFLQSUBFUIHZ4AR4LP5RLJHANCNFSM4H5C6QUA .

alexcrichton commented 5 years ago

@gnzlbg yes this is primarily currently exported from compiler-builtins for us in libstd

@jacobrosenthal this crate should be usable as a cdylib without any dependencies, yes, although you should be able to use this crate itself to define the interface rather than having to copy in theimplementation.

gnzlbg commented 5 years ago

So from your other posts I think we are pretty much all in agreement with the goals and use cases.

alexcrichton commented 5 years ago

Ok, in that case do you think we can close this?

gnzlbg commented 5 years ago

From the new comments, it seems there is disagreement about whether this should only expose a C API, or should only expose a Rust API (or somehow both, in different crates).

burrbull commented 5 years ago

I prefer both API with feature flag, or in different crates.

m1el commented 5 years ago

I'd prefer C API to be a separate crate since the primary purpose of this crate was to support math in Rust no_std code, and not in generic C-compatible languages.

C API has some wrinkles, that's why these C-incompatible interfaces were chosen.

burrbull commented 5 years ago

Yes, Rust in first place.

gnzlbg commented 5 years ago

Can anybody explain me why a Rust API is required to use this in no_std core binaries?

We currently provide a Rust API, so we need to modify the compiler-builtin crate to wrap this Rust API into a C API (e.g. see https://github.com/rust-lang-nursery/compiler-builtins/blob/9eafb677ada5c5c32873ca1c02a47feb64a87bb5/src/math.rs ), so that we can expose it via a Rust API again in libcore / libstd (Rust->C->Rust).

If we were to provide a C API on top of the result, e.g., by calling the libcore math functions, we would have a Rust API wrapped in C wrapped in Rust wrapped in C (C->Rust->C->Rust). That feels a bit nuts.

AFAICT, if this had the C API, we could just compile it, and link it, and that would "just work". LLVM emits platforms' math symbols for the libcore APIs, which if this library was linked, would be defined, and that's it. We end up with just Rust->C, which is what happens in all other platforms, and you have the benefit that if you call C code, that would just do a C->C when using the Rust math library.

AFAICT either way, everything can be inlined, but I must be missing something, since we could just expose here the extern "C" C APIs, and get the best of both worlds, while avoiding the Rust->C->Rust dance.

Schultzer commented 5 years ago

@alexcrichton What are the cons of adding C API? I understand that it's C`ism and old baggage that we rather not carry.

@m1el

C API has some wrinkles, that's why these C-incompatible interfaces were chosen.

Are wrinkles?, for example that in sincos you need to provide two references/pointers?

alexcrichton commented 5 years ago

@gnzlbg

Can anybody explain me why a Rust API is required to use this in no_std core binaries?

That's how it's integrated into compiler-builtins and libstd. If crates want to force usage of libm without resorting to linker trickery using a Rust crate is more ergonomic.

I think it's not a great case but I don't really see why we need to make a C interface as the actual interface of the crate when we could basically just not do that. Adding a C API layer wasn't ever really the intention of this crate, nor was it every really envisioned AFAIK to add a libm.so-style build target. It can certainly be done but I don't think that it should radically change what the crate is already doing.

In terms of cost doesn't this all come out in the wash? It's all inlined into w/e the wrapper is anyway.

AFAICT, if this had the C API, we could just compile it, and link it, and that would "just work"

We want pretty strict control over symbols exposed from compiler-builtins, and making everything no_mangle would break this. We explicitly do not use this crate on all platforms since the native libm is typically faster for various operations. By providing unmangled symbols in compiler-builtins we would force all crates on all platforms to use this implementation.


@Schultzer

What are the cons of adding C API?

I haven't really weighed in on that in this thread. I don't see a problem with adding a C API, the question I think is about defining in terms of C APIs.


FWIW can we split this issue into more actionable ones? It seems sort of a mess to be discussing multiple separate issues in one place since discussions just sort of randomly change over time and it's difficult to follow if ever reading this in the future

gnzlbg commented 5 years ago

@alexcrichton

That's how it's integrated into compiler-builtins and libstd.

Can you point me to the code? The only code I find in compiler-builtins, is essentially turning this Rust API into a C API: https://github.com/rust-lang-nursery/compiler-builtins/blob/9eafb677ada5c5c32873ca1c02a47feb64a87bb5/src/math.rs#L1

And that code doesn't support the APIs returning tuples, because it would probably need to use more complex transformations to recover the C APIs from those.

All of this would be unnecessary, if this crate would stick to the C APIs that compiler-builtins requires in the first place.

gnzlbg commented 5 years ago

We want pretty strict control over symbols exposed from compiler-builtins, and making everything no_mangle would break this. We explicitly do not use this crate on all platforms since the native libm is typically faster for various operations. By providing unmangled symbols in compiler-builtins we would force all crates on all platforms to use this implementation.

I don't understand. If libm would implement the C ABI, we don't have to do anything at all in compiler-builtins AFAICT (that is, do less than what we are doing now). In rust-lang/rust we'd just need to link our libm for the targets in which we want to use it, and that would be it. If we don't link our libm for a target, either there is a system's libm that resolve the symbols (e.g. glibc gets linked with its own libm), or the binary fails to link.

alexcrichton commented 5 years ago

Can you point me to the code?

Yes that's the code I'm aware of. And no due to the point of symbol mangling we would still require basically the same code.

In rust-lang/rust we'd just need to link our libm for the targets in which we want to use it, and that would be it.

We need both ABI changes and name mangling changes. We can't link to unmangled libm names in compiler-builtins as it would export too many symbols. We only export symbols in compiler-builtins which are known to be as fast as the native versions or those which have no native versions.

And that code doesn't support the APIs returning tuples, because it would probably need to use more complex transformations to recover the C APIs from those.

The point of libm was to make the "wasm target just work". Since then people have been getting more and more ambitious about what libm should be doing. I don't really think we should be that ambitious. If we need a tiny tweak to one macro in that crate it really isn't the end of the world.

I still find it weird that we should define things in terms of C ABI and API here. That's not the purpose of the crate, the purpose is to provide well-tested implementations of functions that can be used to polyfill C implementations if necessary. the actual interface/ABI layer happens elsewhere.

gnzlbg commented 5 years ago

If the whole purpose of this crate is to expose the symbols of the C APIs via compiler-builtins, I still don't understand why we don't implement the C APIs here. Why add here a different API, e.g., returning -> (f32, i32), just so that we can re-write it in the compiler-builtin crate to use pointers and out parameters instead ? What's the value in that ? We could just write the C API here, and have less to do in compiler-builtins.

burrbull commented 5 years ago

If the whole purpose of this crate is to expose the symbols of the C APIs via compiler-builtins

The main goal of this crate is to provide math functions for no_std targets. And it already fulfills this goal.

If it is included in core, it is very good, but not critical.

If it can be used as cdylib, it is also good, but not at the price of current functionality.

gnzlbg commented 5 years ago

The main goal of this crate is to provide math functions for no_std targets.

And doing that requires a C API.

If it is included in core, it is very good, but not critical.

libcore should just expose the float methods, and we should require a libm to always be present, either via libstd from the system, or because compiler-builtins is used.

If it can be used as cdylib, it is also good, but not at the price of current functionality.

Agreed. What kind of functionality do you think is impacted by allowing using the library as a cdylib ?

Once we expose the API that compiler-builtins (and therefore libcore) requires, changing crate-type = [ "rlib" ] to crate-type = [ "rlib", "cdylib" ] should just work. Compiler-builtins would continue to handle no_mangle for the rlib like it does now, and for the cdylib, we probably want to just handle symbol names in the build.rs, e.g., prefixing the symbols to _rlibm_... by default, and having some cargo option that allows overriding the standard symbol names.

None of this should impact any functionality when building as an rlib for use in compiler-builtins.

gnzlbg commented 5 years ago

I'm closing this as resolved.

The libstd float methods, like f32::sqrt, generate llvm.sqrt intrinsics, which at some point in the backend after LLVM optimizations get lowered to libm symbols like _sqrtf. I need to be able to use f32::sqrt from libcore, to generate a llvm.sqrt, so that the code can be reasonably optimized, and when the _sqrtf symbol is generated, I need that symbol to be resolved to the appropriate function of the libm crate iff the target doesn't have another libm.

Apparently the intent for this crate is for the methods to be called directly, so I need a different crate for that. Sorry for the confusion.