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

Incorrect infinity behavior for FMA? #2468

Closed
RalfJung opened this issue Aug 6, 2022 · 6 comments · Fixed by #2469
Closed

Incorrect infinity behavior for FMA? #2468

RalfJung opened this issue Aug 6, 2022 · 6 comments · Fixed by #2469

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2022

The following assertion passes on rustc, but fails in Miri:

fn main() {
    let neg_inf: f32 = f32::NEG_INFINITY;
    assert_eq!((-3.2f32).mul_add(2.4, neg_inf), neg_inf);
}

Miri uses the mul_add from apfloat (the Rust port of LLVM's softfloat library) to implement FMA, and somehow apfloat seems to be wrong? Cc @eddyb @workingjubilee

@RalfJung RalfJung changed the title Incorrect NAN behavior for FMA? Incorrect infinity behavior for FMA? Aug 6, 2022
bors added a commit that referenced this issue Aug 6, 2022
implement some missing float functions

With this we support the entire float API surface of the standard library. :)

Also work around #2468 by using host floats to implement FMA.
@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2022

#2469 fixes this for Miri by using host floats rather than soft floats to implement FMA.

But it'd still be good to figure out if this should become an LLVM apfloat bugreport.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2022

What's wild is that the bug only seems to trigger on very particular inputs. This works fine:

    assert_eq!((-3.2f32).mul_add(0.0, neg_inf), neg_inf);
    assert_eq!((-3.2f32).mul_add(1.0, neg_inf), neg_inf);
    assert_eq!((-3.2f32).mul_add(2.0, neg_inf), neg_inf);

But this fails:

    assert_eq!((-3.2f32).mul_add(3.0, neg_inf), neg_inf);

@bors bors closed this as completed in f633537 Aug 6, 2022
@eddyb
Copy link
Member

eddyb commented Aug 6, 2022

FWIW what one could do is try to reproduce this via LLVM constant folding (in Clang ideally, on godbolt's compiler explorer, since Clang versions match LLVM versions - just in case it got fixed at some point).

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2022

Even clang 6 seems to do it correctly. But maybe their constant folding uses a different codepath.

@eddyb
Copy link
Member

eddyb commented Aug 7, 2022

It's basically identical for Rust, when LLVM is doing the const-folding: https://godbolt.org/z/eann1fsMe.

The current impl of LLVM constant-folding llvm.fma.* is:

        case Intrinsic::fma:
        case Intrinsic::fmuladd: {
          APFloat V = C1;
          V.fusedMultiplyAdd(C2, C3, APFloat::rmNearestTiesToEven);
          return ConstantFP::get(Ty->getContext(), V);
        }

So this is likely a port problem, which was easy enough to confirm:

Seems like this bug was there from the start (of the port), I don't really understand how it happened (but obviously I screwed up the otherwise-simple early-return control-flow refactor) - what worries me a bit more is that I heavily relied on tests and there's barely any for FMA (they're all finite AFAICT!).
And it's not like I lost some tests during porting, they only had that many.

This kind of thing is why I was talking to @workingjubilee a while back about potentially starting a new thing from scratch - however, maybe at least we could use some of the IEEE test suites floating around (I think the one I found at some point was IBM's?) and throw them at the existing code to see what else falls out.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 7, 2022

Thanks for investigating! Reported the bug as rust-lang/rust#100233.

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 a pull request may close this issue.

2 participants