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 all stable and safe std f32/f64-methods on FF32 and FF64 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vkahl
Copy link

@vkahl vkahl commented Jan 2, 2021

All these method calls are delegated to the corresponding methods
on the underlying primitive types (f32 or f64). For all methods
returning floats, these are getting converted to Fast<_>.
Of course all of this only works when using std, so I added a
feature gate called "std".

Other minor changes:

  • added .gitignore file to ignore target directory
  • added #[inline(always)] annotations to get, into and operator
    methods
  • updated num-traits version in Cargo.toml
  • reordered the Zero-trait impl so all feature independent impls are on top
  • removed left over (commented out) generic Zero-trait impl
  • applied cargo fmt

All these method calls are delegated to the corresponding methods
on the underlying primitive types (`f32` or `f64`). For all methods
returning floats, these are getting converted to `Fast<_>`.
Of course all of this only works when using std, so I added a
feature gate called "std".

Other changes:
- added .gitignore file to ignore target directory
- added #[inline(always)] annotations to `get`, `into` and operator
  methods
@bluss
Copy link
Owner

bluss commented Jan 2, 2021

If we're going to have some feature development and make this crate something more serious than just an experiment, then we also need to address the top issue of making the FF64/FF32 constructors unsafe - unless we find a good reason not to. What do you think?

@bluss
Copy link
Owner

bluss commented Jan 5, 2021

On further thought, there are (after the unsafe question) some methods here that I'd prefer to not implement. For example .mul_add(). That operation is not going to be performed using the fast-math flags, so implementing it is misleading. This might more or less be extended to most methods(!) It would be bad to give the user the impression that something is enabled that is not.

@vkahl
Copy link
Author

vkahl commented Jan 8, 2021

That makes a lot of sense. I was mostly going for a quick drop-in replacement for normal float types that uses the flag whenever possible. By now I figured out that it makes more sense to properly implement Float from num-traits to achieve that (which is easily done using the num-derive crate).

@scottmcm
Copy link

scottmcm commented Feb 4, 2021

then we also need to address the top issue of making the FF64/FF32 constructors unsafe - unless we find a good reason not to. What do you think?

With nightly rust on LLVM 11 now (soon to be 12), the freeze instruction is available. Maybe you'd be interested in making a PR to rustc to expose that, @vkahl? That would allow Fast<T> to be safe by making it be struct Fast<T> { inner: MaybeUninit<T> }, with a trivial constructor from arbitrary T and the Fast<T>: Into<T> doing the freeze so that if bad things had happened it'd just be unspecified, not potentially-UB.

(Note that I don't think just making the constructors unsafe is the right way forward -- Fast(0.0) should presumably be sound, but Fast(0.0) / Fast(0.0) gives a Fast(NAN). Thus the idea to instead allow a Fast to contain poison, and just keep that poison from escaping to safe code.)

This might more or less be extended to most methods(!)

+1 to this. Probably shouldn't offer a "fast" tan and such if they're just the same ones as the normal type.

I think mul_add could be exposed as fast_add(fast_mul(a, b), c), though, because the fast flag includes contract, which allows combining the two if profitable.

@bluss
Copy link
Owner

bluss commented Feb 4, 2021

I also notice that impl_assignop is completely broken (uses + for every op). This has just been a crate useful for some experiments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants