This is an archive of the discontinued LLVM Phabricator instance.

Make uitofp and sitofp defined on overflow.
ClosedPublic

Authored by efriedma on Jun 5 2018, 6:42 PM.

Details

Summary

IEEE 754 defines the expected result on overflow. As far as I know, hardware implementations (of f16), and compiler-rt (__floatuntisf) correctly return +-Inf on overflow. And I can't think of any useful transform that would take advantage of overflow being undefined here.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jun 5 2018, 6:42 PM

This is reversing part of D5603 / rL219542 . I see there was some controversy in the post-commit thread, although it was for the FP -> int direction. Let me add those commenters as reviewers in any case.

For reference, IEEE754, section 7.4 says:
"The overflow exception shall be signaled if and only if the destination format’s largest finite number is exceeded in magnitude by what would have been the rounded floating-point result (see 4) were the exponent range unbounded. The default result shall be determined by the rounding-direction attribute and the sign of the intermediate result as follows:
a) roundTiesToEven and roundTiesToAway carry all overflows to ∞ with the sign of the intermediate result."

And the C standard, section 6.3.1.4 says:
"When a value of integer type is converted to a real floating type, if the value being converted can be represented exactly in the new type, it is unchanged. If the value being converted is in the range of values that can be represented but cannot be represented exactly, the result is either the nearest higher or nearest lower representable value, chosen in an implementation-defined manner. If the value being converted is outside the range of values that can be represented, the behavior is undefined."

That language (like the LangRef regardless of what we decide here) probably needs refinement because there is no "outside of the range" of infinity?

Note: I moved/corrected the tests at rL334107 (shouldn't have been under instcombine in the first place). Please rebase with full context for the record.

Adding some more possible reviewers (people that commented on the recent junk-in-the-ftrunc changes): D46135.

The C standard language is weird because it was written before IEEE fp was used universally; some formats didn't have infinity.

efriedma updated this revision to Diff 150181.Jun 6 2018, 12:33 PM

Rebased, and uploading with full context.

spatel accepted this revision.Jun 6 2018, 3:38 PM

I agree that this is a good change, so LGTM. But we should wait a bit for more comments in case anyone else sees potential downside.

docs/LangRef.rst
3289 ↗(On Diff #150181)

Nit - I'd find it easier to read if we rewrote as:

``uitofp (CONSTANT to TYPE)``
    Convert an unsigned integer constant to floating-point. 
    CONSTANT must be a scalar or vector integer type. 
    TYPE must be a scalar or vector floating-point type. 
    If CONSTANT is a vector, TYPE must be a vector with the same number of elements.
This revision is now accepted and ready to land.Jun 6 2018, 3:38 PM
nikic added a subscriber: nikic.Jun 7 2018, 2:30 AM

Rust currently generates additional code for u128 to f32 casts to work around this problem (see https://github.com/rust-lang/rust/blob/c131bdcaff68d35f96e954baac4340206779335f/src/librustc_codegen_llvm/mir/rvalue.rs#L795). Making this defined would allow directly using uitofp, so this would be a welcome change.

This revision was automatically updated to reflect the committed changes.