Skip to content

Use f{32|64}::powi instead of Typenum::Pow::powi.#193

Merged
iliekturtles merged 1 commit into
masterfrom
powi-fix
Jun 23, 2020
Merged

Use f{32|64}::powi instead of Typenum::Pow::powi.#193
iliekturtles merged 1 commit into
masterfrom
powi-fix

Conversation

@iliekturtles

Copy link
Copy Markdown
Owner

In Rust 1.45 the upgrade to LLVM 10.0 changes the behavior of
f{32|64}::powi on Windows. This commit changes the Quantity::powi
implementation to simplify down to f{32|64}::powi instead of using
Typenum::Pow::powi which uses a less precise algorithm. Resolves #192.

Reviews welcome. Will plan to merge in a couple days.

@adamreichold

Copy link
Copy Markdown
Contributor

I understand this is the faster solution, but I do wonder whether comments like https://docs.rs/typenum/1.12.0/src/typenum/type_operators.rs.html#103 still apply. Meaning that we should probably fix this in the typenum crate by making them forward to {f32,f64}::powi instead of doing it here (and for everywhere else typenum is used)?

@adamreichold

Copy link
Copy Markdown
Contributor

I understand this is the faster solution, but I do wonder whether comments like https://docs.rs/typenum/1.12.0/src/typenum/type_operators.rs.html#103 still apply.

To answer my own question, it does seem to apply indeed, i.e. {f32,f64}::powi are not available in no_std environments and core::instrinsics::powi{f32,f64} are unstable. So this does look the best approach. Sorry for the noise...

Comment thread src/system.rs Outdated
dimension: $crate::lib::marker::PhantomData,
units: $crate::lib::marker::PhantomData,
value: $crate::typenum::Pow::powi(self.value, e),
value: self.value.into_conversion().powi(E::to_i32()).value(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably just a lack of understanding, but looking at e.g. the implementation of recip, why is the trait bound

V: $crate::num::Float

and the implementation

value: self.value.powi(E::to_i32()),

not applicable here?

@iliekturtles iliekturtles Jun 22, 2020

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I thought I tested this when I did the original fix, but I must have been pretty distracted because I checked it again and it works. I just pushed the fix and am waiting on the build to finish. Thanks for the review!

In Rust 1.45 the upgrade to LLVM 10.0 changes the behavior of
`f{32|64}::powi` on Windows. This commit changes the `Quantity::powi`
implementation to simplify down to `f{32|64}::powi` instead of using
`Typenum::Pow::powi` which uses a less precise algorithm. Resolves #192.
@iliekturtles iliekturtles merged commit 3f92425 into master Jun 23, 2020
@iliekturtles iliekturtles deleted the powi-fix branch January 7, 2021 21:46
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.

Regression in powi on Windows beta/nightly

2 participants