Skip to content

[SIL Diagnostics] Add warnings for detecting imprecision in conversions from larger Floats to smaller Floats. #16738

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

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

ravikandhadai
Copy link
Contributor

Add tests to check the diagnositcs and correctness of the constant
folding on FPTrunc operations

@ravikandhadai
Copy link
Contributor Author

@stephentyrone I have implemented the strategy for detection underflows and overflows during floating-point literal conversion as discussed here: https://forums.swift.org/t/rfc-on-compile-time-diagnosis-of-erroneous-floating-point-operations/12125/11. Let me know your comments.

@ravikandhadai
Copy link
Contributor Author

@swift-ci please smoke test


WARNING(warning_float_trunc_hex_inexact, none,
"'%0' cannot be precisely represented by %1"
"%select{| or by smaller floating-point types}2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Is "or by smaller floating-point types" necessary to make the essential point here? I'd have to work it out on paper, but I think it's technically possible for, say, a subnormal value to be non-representable by an IEEE floating-point type but representable by a hypothetical non-standard smaller floating-point type that has more bits for the significand.

Larger point: Can this warning be silenced by some syntax (say, parentheses, or an explicit casting notation such as T(0x1p2)) without deleting the extra digits? I think it may be useful if the idea is that I want to be able to paste a hex float with much larger precision with the expectation that it will be truncated appropriately for whatever type. Indeed, that is how it works for decimal float literal notation and, even if not the default behavior, should not be something that becomes effectively prohibited for hex floats with an unsilenceable warning.

Copy link
Contributor Author

@ravikandhadai ravikandhadai May 23, 2018

Choose a reason for hiding this comment

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

@xwu Here is some explanation for some of the points you raise:

Is "or by smaller floating-point types" necessary to make the essential point here?

When you call initializers explicitly e.g. "Float(some_literal)" -- first the literal is implicitly converted to "Double" and then to "Float". So if there is a warning during the implicit conversion to "Double", the warning would refer to the type "Double" which could be a bit surprising to the user since he/she never wrote it. Btw, this problem is more general and not specific to Floating point conversion. A better diagnostic that takes in account the context of the implicit conversion is a bit more involved, and is an on going work. For the time being I think it is better to have some qualifier (even though redundant in some cases) that indicates that the type used in diagnostics and the one written by the user may be different. I have made a recent change to the PR in which the error message reads slightly differently: "literal loses precision during (implicit) conversion to Double" , where the word (implicit) is included for warnings generated during implicit conversions but otherwise omitted.

... non-representable by an IEEE floating-point type but representable by a hypothetical non-standard smaller floating-point type that has more bits for the significand.

The diagnostics added here is very specific to the builtin floating-point types supported by Swift: Float, Double and Float80 (on x86 architectures). They follow IEEE 754 standard. The diagnostics won't be emitted for any other user-defined Floating point types.

Can this warning be silenced by some syntax (say, parentheses, or an explicit casting notation such as T(0x1p2)) ?

The warning will be produced if and only if a floating-point type (Float, Double or Float80) is assigned or initialized with a hex float that is bound to lose precision when represented in the type. The warning wouldn't be raised if the literal is first initialized to a variable of proper type and later cast to a smaller type. So one way to eliminate the warning is to bind the literal to a large enough floating-point type and later convert it explicitly to a smaller type e.g.

let x: Float = 0x1.00000000001p23 // warning will be produced
let x = Float(0x1.00000000001p23) // warning will be produced

let x: Double =0x1.0000000001p23
let y = Float(x) // no warning

Indeed, that is how it works for decimal float literal notation and, even if not the default behavior, should not be something that becomes effectively prohibited for hex floats with an unsilenceable warning.

We considered adding this warning for hex floats since the reason for using hex float notation is to be precise and avoid any imprecision that may happen with decimal notation. If a truncation is desired, it could be done explicitly as illustrated by the last example presented above.

uint64_t truncSignificand = bitWidthDecrease > 0 ?
(srcSignificand >> bitWidthDecrease) : srcSignificand;

// Compute the significand bits lost due to denormal form. Note that the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps best to standardize on the modern term "subnormal" (used elsewhere in Swift) rather than "denormal"?

@ravikandhadai ravikandhadai force-pushed the FPTruncPR branch 2 times, most recently from 8607fd9 to faaaeaa Compare May 23, 2018 01:01
@ravikandhadai
Copy link
Contributor Author

@swift-ci please smoke test

@xwu
Copy link
Collaborator

xwu commented May 23, 2018

When you call initializers explicitly e.g. "Float(some_literal)" -- first the literal is implicitly converted to "Double" and then to "Float". So if there is a warning during the implicit conversion to "Double", the warning would refer to the type "Double" which could be a bit surprising to the user since he/she never wrote it. Btw, this problem is more general and not specific to Floating point conversion. A better diagnostic that takes in account the context of the implicit conversion is a bit more involved, and is an on going work. For the time being I think it is better to have some qualifier (even though redundant in some cases) that indicates that the type used in diagnostics and the one written by the user may be different. I have made a recent change to the PR in which the error message reads slightly differently: "literal loses precision during (implicit) conversion to Double" , where the word (implicit) is included for warnings generated during implicit conversions but otherwise omitted.

This behavior is going away imminently, given that the Swift Evolution proposal was approved. There is no version of Swift which will be released in which both this warning and the behavior you describe would occur, and I think the diagnostics would benefit from the clarity possible from not trying to contort the language used to help users with let x: Float = 4e38 also to accommodate the very different issue that's going on with let y = Float(4e38) that will be going away shortly.

There is another, related double-rounding issue with float literals being first used to create a Float80 value (or Double on platforms without Float80), then to the assigned type, but that's also something that is really another kettle of fish from diagnostics for let x: Float = 4e38.

We considered adding this warning for hex floats since the reason for using hex float notation is to be precise and avoid any imprecision that may happen with decimal notation. If a truncation is desired, it could be done explicitly as illustrated by the last example presented above.

I understand the motivation and would even agree with it, but my point is that, IMO, the warning needs to be silenceable more ergonomically than requiring two statements like the example you provide above.

"conversion of '%0' to %1", (StringRef, Type, bool, bool))

WARNING(warning_float_trunc_hex_inexact, none,
"'%0' loses precision during%select{| implicit}2 conversion to %1",
Copy link
Collaborator

@xwu xwu May 23, 2018

Choose a reason for hiding this comment

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

I like this better! What do you think of standardizing the language used in these warnings? Right now, the overflow warnings begin with "overflow: [explanation]" and the two precision loss warnings use totally different language. Maybe:

  • '%0' overflows to %select{|-}3inf during%select{| implicit}2 conversion to $1
  • '%0' underflows to %select{|-}30.0 during%select{| implicit}2 conversion to $1
  • '%0' loses precision during%select{| implicit}2 conversion to %1
  • '%0' overflows to %select{|-}1inf because its magnitude exceeds the limits of a float literal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the warning needs to be silenceable more ergonomically

Can you please elaborate on what that more elegant "ergonomic" syntax for disabling the warning would be in your opinion? If you can provide some examples where you do not want to see the warning it would be very helpful.

(There is also the technical difficulty of whether we can detect those "erogonomic" subtleties at the SIL diagnostic level. Note that the AST information that we recover at SIL diagnostics level is limited. E.g. let x: Float = 1.0p26 ,let x = (1.0p26 as Float), let x = Float(1.0p26 ) all look pretty much the same at the SIL level even if we get the AST information from the SIL instructions, because of the way AST nodes are associated with SIL instructions. The future changes in Swift will be helpful and has the potential to make the diagnostics better. But as of now I am trying to make the diagnostics as much clear as possible without relying on the future changes.)

But, let me know what kind of warning disabling you would like, and I will see if it can be detected and done with reasonable overheads and implementation at the SIL level.

Copy link
Contributor Author

@ravikandhadai ravikandhadai May 23, 2018

Choose a reason for hiding this comment

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

'%0' overflows to %select{|-}3inf during%select{| implicit}2 conversion to $1
'%0' loses precision during%select{| implicit}2 conversion to %1
'%0' overflows to %select{|-}1inf because its magnitude exceeds the limits of a float literal

I am fine with all three of these messages. I will make those changes.

'%0' underflows to %select{|-}30.0 during%select{| implicit}2 conversion to $1

I am not very sure about this. As far as I understand, 'underflow' is said to happen the moment a value becomes subnormal i.e, the exponent in the normal form falls below minimum representable value. But the warning is emitted only when this underflow results in loss of precision more than what would happen without an underflow.
In other words, 3 conditions must hold for the warning: (a) degeneration to subnormal form (underflow) and (b) loss of precision during conversion (inexactness) and (c) loss of precision more than what would happen with unbounded exponent.
(For more information see this discussion: https://forums.swift.org/t/rfc-on-compile-time-diagnosis-of-erroneous-floating-point-operations/12125/11).

It has been difficult for me to come up with a succinct, good message here. But, I feel that the current message provides more information to a user than just that an "underflow" happened. It is meant to convey that there is more imprecision because of the underflow than what they would expect from the truncation.

I would think that something like the following would be better:
"'%0' loses precision due to tininess during%select{| implicit}2 conversion to $1"

Btw, although I would like to print the new value (in your example -30.0 ), it may not convey a loss in precision. E.g. try in REPL

(swift) let _: Float = 5.87747e-39
:1:16: warning: precision loss due to tininess during conversion of '5.87747e-39' to 'Float'
let _: Float = 5.87747e-39
^
// _ : Float = 5.87747e-39

Here the new and the old values look identical (because of the way they are printed) but there has been a loss in precision. I am wondering how to print the value, perhaps we could go on enumerating digits until it starts differing from the literal written by the user. But not sure if it is worth the effort.

Copy link
Contributor Author

@ravikandhadai ravikandhadai May 23, 2018

Choose a reason for hiding this comment

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

@xwu Btw, if you would like any changes to logic/conditions that are used to decide on when to warn on underflows, please post in the Swift forum discussion thread given above. That way other experts in the forum can also be brought into the discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to answer for @xwu, but I think we need a way for a user to indicate locally that loss of precision is ok for a specific literal, without disabling the warning by a flag at file or module scope.

Copy link
Contributor Author

@ravikandhadai ravikandhadai Jun 14, 2018

Choose a reason for hiding this comment

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

@stephentyrone @xwu Thanks for the feedback. I have pushed a new commit in which the warnings are disabled if the floating-point literal is truncated using an explicit initializer.
E.g.

$ Float(5.87747e-39) // no warning
$ let f: Float = 5.87747e-39
warning: '5.87747e-39' underflows and loses precision during conversion to 'Float'

@ravikandhadai
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@stephentyrone stephentyrone left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.

floating-point literals to variables of floating-point type.
@ravikandhadai
Copy link
Contributor Author

@swift-ci please test and merge

@shahmishal
Copy link
Member

@swift-ci please clean test macOS

@ravikandhadai
Copy link
Contributor Author

@swift-ci please test and merge

@swift-ci swift-ci merged commit eaa3d2c into swiftlang:master Jun 19, 2018
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 this pull request may close these issues.

6 participants