-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Conversation
@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. |
@swift-ci please smoke test |
include/swift/AST/DiagnosticsSIL.def
Outdated
|
||
WARNING(warning_float_trunc_hex_inexact, none, | ||
"'%0' cannot be precisely represented by %1" | ||
"%select{| or by smaller floating-point types}2", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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"?
8607fd9
to
faaaeaa
Compare
@swift-ci please smoke test |
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 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
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. |
include/swift/AST/DiagnosticsSIL.def
Outdated
"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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
@swift-ci please smoke test |
There was a problem hiding this 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.
@swift-ci please test and merge |
@swift-ci please clean test macOS |
@swift-ci please test and merge |
Add tests to check the diagnositcs and correctness of the constant
folding on FPTrunc operations