Skip to content
This repository was archived by the owner on Apr 28, 2025. It is now read-only.

Add tests against MPFR for ilogb and ilogbf #398

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jan 5, 2025

No description provided.

@tgross35 tgross35 force-pushed the tgross35/ilogb-tests branch from cabf548 to fad3f86 Compare January 5, 2025 02:27

fn run(this: &mut Self::MpTy, input: Self::RustArgs) -> Self::RustRet {
this.assign(input.0);
this.get_exp().unwrap_or(i32::MIN)
Copy link
Member

Choose a reason for hiding this comment

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

get_exp follows frexp's rules for determining which exponent to use, which is to pick v = m * 2^e where 0.5 <= |m| < 1.0. ilogb by contrast uses e = floor(log2(|v|)) (without rounding errors) which is equivalent to 1.0 <= |m| < 2.0.
so, you need get_exp().map(|v| v - 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks. Updated this and added your explanation as a comment.

@tgross35 tgross35 force-pushed the tgross35/ilogb-tests branch from fad3f86 to 5b76943 Compare January 6, 2025 03:39
@tgross35 tgross35 changed the title [WIP] Add tests against MPFR for ilogb and ilogbf Add tests against MPFR for ilogb and ilogbf Jan 6, 2025
@tgross35 tgross35 marked this pull request as ready for review January 6, 2025 03:39
// one to scale the significand to `1.0 <= |m| < 2.0`.
this.get_exp().map(|v| v - 1).unwrap_or_else(|| {
if this.is_infinite() {
i32::MAX
Copy link
Member

Choose a reason for hiding this comment

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

some microcontrollers have 16-bit c_int (AVR? icr if the rust version does but iirc some C compilers do), so you should probably use c_int here rather than i32

Copy link
Member

@programmerjake programmerjake Jan 6, 2025

Choose a reason for hiding this comment

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

or are you assuming the tests never are compiled on 16-bit c_int targets? (since idk if mpfr can compile for those targets)

Copy link
Contributor Author

@tgross35 tgross35 Jan 6, 2025

Choose a reason for hiding this comment

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

Yeah this isn't accurate but the source needs to be fixed for this and other routines that should be c_int

const FP_ILOGBNAN: i32 = -1 - 0x7fffffff;
const FP_ILOGB0: i32 = FP_ILOGBNAN;
#[cfg_attr(all(test, assert_no_panic), no_panic::no_panic)]
pub fn ilogb(x: f64) -> i32 {

I doubt MPFR would build on 16-bit targets, I can't even get it to compile for i586 or any Windows targets (might be possible but I haven't spent a lot of time here yet). I have a branch that serializes test input and MPFR results to a file and can run tests against that, that is my loose plan for how to get coverage on these targets without building mpfr/musl.

Copy link
Member

Choose a reason for hiding this comment

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

well, upstream mpfr claims to compile on i686 mingw and debian has a version for "i386", so i expect it to work for those; AVR is more questionable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MinGW should be possible, I just ran into a lot of difficulty getting the correct dependencies and paths set up via msys on GHA. I guess I need to revisit i586; I thought there were build failures but it looks like it is our implementation that is dropping the sign #404. Probably related to how we unconditionally use x87 https://github.com/rust-lang/libm/blob/ff15e465d6d4beb81e686fdfdeb40388b474bb3e/src/math/arch/i586.rs.

@tgross35 tgross35 force-pushed the tgross35/ilogb-tests branch from 5b76943 to 1b52164 Compare January 6, 2025 04:59
@tgross35 tgross35 merged commit ba4bc97 into rust-lang:master Jan 6, 2025
30 checks passed
@tgross35 tgross35 deleted the tgross35/ilogb-tests branch January 6, 2025 06:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants