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

Bad floating-point "optimizations" #43070

Closed
tydeman mannequin opened this issue Oct 20, 2019 · 14 comments
Closed

Bad floating-point "optimizations" #43070

tydeman mannequin opened this issue Oct 20, 2019 · 14 comments
Labels

Comments

@tydeman
Copy link
Mannequin

tydeman mannequin commented Oct 20, 2019

Bugzilla Link 43725
Version 8.0
OS Linux
CC @DougGregor,@zygoloid

Extended Description

Is there a way to disable ALL "optimizations" related to floating-point?
My testing has shown that clang does the following:

  1. * x  => x
  0. * x  => 0.
  x / 1.  => x
  x / x   => 1.
  0. / x  => 0.
  x + 0.  => x
  x - 0.  => x
  x - x   => 0.

They are all invalid "optimizations" if x is a signaling NaN.
In addition, some are invalid if x is a signed zero or infinity.
In the above, 'x' can be either a variable or a constant.
It might matter if 'x' is float, double, or long double.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 23, 2019

Currently clang and llvm assume that floating point generates no side effects. There is work in progress to teach llvm and clang to be strict with floating point so many optimizations don't fire.

If you want to follow the progress the tickets at reviews.llvm.org tend to have "[FPEnv]" in the name somewhere, and this is sometimes true for the mailing lists as well. See also the "constrained floating point intrinsics" in the Language Reference: https://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics

@tydeman
Copy link
Mannequin Author

tydeman mannequin commented Oct 23, 2019

I am glad to see that some work is being done on this issue.
http://www.tybor.com/tflt2int.c
is a good test case for
‘llvm.experimental.constrained.fptoui’ Intrinsic
‘llvm.experimental.constrained.fptosi’ Intrinsic

However, it appears to me that those intrinsics would not work for integer bit fields with respect to raising invalid for FP numbers too big to fit.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 23, 2019

It is a work in progress. I haven't looked at bitfields, but this ticket may get us closer:

"Add constrained int->FP intrinsics"
https://reviews.llvm.org/D69275

@tydeman
Copy link
Mannequin Author

tydeman mannequin commented Jul 4, 2020

Command line options for clang to try to get good floating-point conformance
clang command line options to try to get conformance to IEEE-754 and Standard C.

@tydeman
Copy link
Mannequin Author

tydeman mannequin commented Jul 4, 2020

With those command line options, the test case
http://www.tybor.com/tflt2int.c
still is getting 183 failures.

I tried those options after reading bug 8100.
I tried what it suggested and reporting a bug in that 'fix'.

@CAD97
Copy link

CAD97 commented Jan 24, 2023

LLVM assume[s] that floating point generates no side effects.

For what it's worth, I don't think this is enough to justify turning x * 1.0f into x ([alive2]). If x is a signaling NaN, the IEEE standard specifies that the operation returns a quiet NaN after raising the invalid operation exception. If you mask fp exceptions such that no flags are ever set, this is still the specified behavior.

If you want to justify this transformation, you need to also treat all NaNs as quiet NaNs. This is a reasonable simplifying assumption for LLVM to make, but it should be made explicitly.

@rotateright
Copy link
Contributor

The Language Reference says this about the default FP env:
"No floating-point exception state is maintained in this environment. Therefore, there is no attempt to create or preserve invalid operation (SNaN) or division-by-zero exceptions."

https://llvm.org/docs/LangRef.html#floatenv

Suggestions welcome if that text is not clear. Otherwise, I think we can close this bug now. "Strict" FP is supported for multiple targets. If anything is still broken in that mode, there should be narrower bugs filed with examples to show a problem.

@efriedma-quic
Copy link
Collaborator

The only thing that's a little weird for non-strict FP is that if we optimize x * 1.0 -> x, that effectively means fmul can produce an SNaN. If we're okay with that, it's probably worth documenting explicitly. (If we want to avoid that, we can transform x * 1.0 -> llvm.canonicalize(x).)

@CAD97
Copy link

CAD97 commented Jan 27, 2023

This was in fact exactly my point; if reducing x * 1.0 -> x is acceptable for nonstrict FP, this should be documented more explicitly. This is not just a case of not preserving an invalid operation (SNaN) exception; it's actually changing the value from the IEEE-defined result. In fact, all of the OP's examples are not about exceptions; they're all operations that should produce some qNaN when x is sNaN. Any other FP nonstrictness is not the intended domain of this specific issue.

FWIW,

  • It's currently the case that a constrained fmul with !"round.tonearest", !"fpexcept.ignore" (i.e. the default environment settings) will not perform this contraction (nor to llvm.canonicalize). [godbolt]
  • Alive2 seems to not have full strictfp support, as it reports the contraction as valid even for the strictfp version. [godbolt]
  • I tried to test llvm.canonicalize to see if it lowered differently and got an instselect error instead. Cannot select llvm.canonicalize.f64 #32650

Personally, I'm perfectly agreeable with a resolution of clarifying that non-strictfp ignores sNaN, treating all NaN as qNaN, so long as that choice is documented.

Additional but unnecessary nice-to-haves:
  • A commitment that treating all NaN as quiet is the extent of the sNaN weakening; that it's not actually just UB to operate on sNaN outside strictfp.
  • Ensuring most optimizations still work under strictfp when supplying the default rounding mode and exception behavior.
  • Being able to use default-modal strict floating point intrinsics without annotating functions/calls with strictfp.
    • Having to respect full strictfp semantics for function calls is a significant expense solely to preserve IEEE semantics for sNaN.
  • Being able to tag floating-point operations directly with strictfp (without tagging the rest of the world) to get sNaN-respecting (but still exception-ignoring) semantics.
  • Clarification on exactly how strict strictfp is; is it just about respecting the floating-point environment, or is it fully "match the target's FPU" level strict and guarantees that the NaN payload matches what the target FPU does (if the target has a deterministic behavior)?
  • Clarification on NaN semantics more generally; what operations preserve NaN payload bits, and are the NaN payload bits from a NaN-producing operation arbitrary-but-consistent (a la implied freeze) or fully nondeterministic?
    • This as an axis of control for strictfp, or being able to use freeze for this purpose.
    • Note that arbitrary-but-consistent makes floating-point operations impure, in that it's not valid to duplicate computation anymore, since the separate copies may make different decisions inconsistent with each other.
  • Promoting the constrained floating point intrinsics out of the experimental namespace.

I'll probably make a fresh issue for all of the NaN clarifications that I'm aware of us (Rust) wanting from LLVM, since it's not just the sNaN semantics covered by this issue.

@Muon
Copy link

Muon commented Jan 31, 2023

Regarding x * 1.0 -> x, I believe the offending transformations are in simplifyFMAFMul() and visitFMul(), run in instcombine. This has nothing to do with exception flags. It is a direct violation of IEEE 754, since an sNaN can never be the result of an addition, subtraction, multiplication or division.

I suspect the reason alive2 considers this valid is because the SMT-LIB floating-point theory does not differentiate between different kinds of NaNs.

Also @CAD97 your last example link points to the alive2 run.

@rotateright
Copy link
Contributor

To be clear, this is about more than "x * 1.0" alone. As noted in the original problem report, we allow several FP simplifications that ignore converting SNaN to QNaN.

Here's an incomplete sampling of FP transforms that are allowed in LLVM's default FP environment:
https://alive2.llvm.org/ce/z/igb55y

Proposal to update the LangRef text to make it clearer that those are expected transforms:

"The default LLVM floating-point environment assumes IEEE754-compliant operations except that floating-point instructions do not have side effects. Results assume the round-to-nearest rounding mode. No floating-point exception state is maintained in this environment. Therefore, there is no guarantee that exceptions or their associated state (signaling NaN values) are created, cleared, or preserved. All NaN values are treated as quiet values, but otherwise they are propagated according to IEEE754 rules.

The benefit of this exception-free assumption is that floating-point operations may be speculated freely without any other fast-math relaxations to the floating-point model."

@Muon
Copy link

Muon commented Feb 1, 2023

There seems to be some confusion about how exceptions and signaling NaNs relate there. An sNaN is a NaN value that always causes the "invalid operation" exception to be raised when you try to operate on it, not a kind of NaN that is produced by things signaling exceptions. It's not associated state, and it can only be created by directly specifying it in the source code or casting the right value from an integer. (Under default exception handling, IEEE 754 mandates that a float-producing operation that signals an invalid operation exception must deliver a qNaN.) Note that the invalid operation exception is also raised whenever you compute 0/0 or ∞ - ∞, and it's the default handling of that exception that produces the qNaN.

@rotateright
Copy link
Contributor

I'm not sure how to word it better, but here's an attempt:
https://reviews.llvm.org/D143074
I didn't find Phab IDs for @Muon or @CAD97 , so please add yourselves there if you'd like to comment on the patch.

rotateright added a commit that referenced this issue Feb 15, 2023
Make it explicit that SNaN is not handled differently than
QNaN in the LLVM default floating-point environment.

Note that an IEEE-754-compliant model disallows transforms
like "X * 1.0 -> X". That is because math operations are
expected to convert SNaN to QNaN (set the signaling bit).

But LLVM has had those kinds of transforms from the beginning:
https://alive2.llvm.org/ce/z/igb55y

We should be IEEE-754-compliant under strict-FP (the logic is
implemented with a helper named canIgnoreSNaN()), but I don't
think there is any demand to do that with default optimization.

See issue #43070 for earlier draft/discussion about this change.

Differential Revision: https://reviews.llvm.org/D143074
@rotateright
Copy link
Contributor

We made the documentation clearer, so closing this. If there are still problems, please file new issues with specific examples/requests.

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

No branches or pull requests

6 participants