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

apfloat bug: incorrect handling of infinities in FMA #100233

Closed
RalfJung opened this issue Aug 7, 2022 · 3 comments · Fixed by #113843
Closed

apfloat bug: incorrect handling of infinities in FMA #100233

RalfJung opened this issue Aug 7, 2022 · 3 comments · Fixed by #113843
Labels
A-floating-point Area: Floating point numbers and arithmetic

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2022

The Rust port of LLVM's apfloat has a bug in its FMA (fused-multiply-add) implementation. See rust-lang/miri#2468 (comment) for details, but basically it fails this assertion:

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

AFAIK we don't use apfloat FMA anywhere (Miri stopped using them to work around this bug), but this should still be tracked, and will need to be fixed before we can allow FMA in const.

@eddyb
Copy link
Member

eddyb commented Aug 8, 2022

Copying the relevant parts here (it's a clumsy mistake I made when porting):

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.

@eddyb
Copy link
Member

eddyb commented Jul 11, 2023

... (it's a clumsy mistake I made when porting):

Turns out it's not that simple. I'm not actually doing the same thing as the C++ code, but instead I'm bypassing their multiplySpecials distinction entirely and using the normal multiplication facilities.

So it's more likely a case of missing some subtler special case because of the odd refactor.

(I'm not sure entirely why I did it that way, but FMA is at least special in one way: it needs more precision, and [Limb; 2] instead of [Limb; 1], which multiplySignificand handles in the C++ version by whether addend is NULL, i.e. it has FMA and non-FMA modes, but I inlined multiplySignificand to remove the dynamic choice which was handled in the C++ code with dynamic heap allocations)


EDIT: yupp, the bug is that status == Status::OK should be status != Status::INVALID_OP, because the Rust version can set more flags than the C++ one could - OTOH, now I have a bunch of FMA testcases that all trip the same assertion in sig::add_or_sub (assert(!carry); in the original C++ code).

EDIT2: it was a bit tricky to enable C++ assertions in my custom build, but it does repro:

rustc_apfloat-fuzz: /home/eddy/Projects/rustc_apfloat/target/debug/build/rustc_apfloat-fuzz-a919140aae98d3e2/out/llvm-project-f3598e8fca83ccfb11f58ec7957c229e349765e3/
  llvm/lib/Support/APFloat.cpp:1482: llvm::lostFraction llvm::detail::IEEEFloat::addOrSubtractSignificand(const llvm::detail::IEEEFloat &, bool):
    Assertion `!carry' failed.

My best guess for that one is that it likely got fixed in llvm/llvm-project@e62555c.

EDIT3: ignore the status == Status::OK comment, the real problem/solution is subtler: FMA of the form finite * finite + infinite must ignore the finite * finite multiplication entirely, instead of letting it overflow (and that seems to deal with all the FMA mismatches found by fuzzing).

EDIT4: the assertion I mentioned earlier might be the same bug as #93224.

@eddyb
Copy link
Member

eddyb commented Jul 13, 2023

I posted a longer update to #55993 (comment) regarding the progress with the whole rustc_apfloat situation, but of note for this issue is that the WIP repo already has this bug fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants