Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

`std.math.FloatingPointControl` fixes #1208

Merged
merged 3 commits into from

5 participants

@denis-sh

std.math.FloatingPointControl is drammatically incorrect without this pull.

denis-sh added some commits
@denis-sh denis-sh [whitespace] Fix spaces in `std.math.FloatingPointControl` example an…
…d unittest.
9bf70d5
@denis-sh denis-sh Fix `std.math.FloatingPointControl` two issues.
1. Do not call `setControlState` in destructor if not initialized (previously results in enabling all exceptions).
2. Call `initialize` on setting rounding mode (previously not called).
99e3440
@denis-sh denis-sh [docs] Improve `std.math.FloatingPointControl` example. 6fb40fb
@donc donc was assigned
@andralex
Owner

assigned @donc

@don-clugston-sociomantic don-clugston-sociomantic commented on the diff
std/math.d
((14 lines not shown))
fpctrl.enableExceptions(FloatingPointControl.severeExceptions);
- double y = x*3.0; // will generate a hardware exception, if x is uninitialized.
- //
+ // This will generate a hardware exception, if x is a
+ // default-initialized floating point variable:
+ real x; // Add `= 0` or even `= real.nan` to not throw the exception.

Yes, that it what it does, but it's a bug -- it's not supposed to generate an exception.
Actually whether it throws an exception or not is different depending on whether you are on an AMD or Intel processor.

If I don't miss something it is a documented behavior. But also I still don't understand how exactly it behaves from you comment. Could you please clarify the documentation (e.g. defining platform depending behavior)?

real x;
should not generate an exception. An exception should only be generated if x is actually used. But the compiler is incorrectly inserting a load instruction when it initializes the variable. This is a compiler bug, and it's my fault, I've never got around to fixing it. It makes the feature practically useless at the moment. The funny thing is that Intel processors do not generate exceptions when they load 80-bit signalling NaNs, but AMD processors do, so the bug doesn't always have an effect. (I don't think the difference between the processors is intentional, I think AMD just didn't read Intel's docs carefully enough). Both processors will generate an exception as soon as you do an operation on a signalling NaN.

Being an Intel user I didn't know about such AMD behavior. The comment "This will generate..." was about the second line (as for me it was "obviously" a source of exception).

Could you add your comment to FloatingPointControl's documentation? With addition of "Defining platform depending behavior" section and "Bugs" section? It will really help a lot for people to understand what really happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@don-clugston-sociomantic

Looks OK apart from the issue I mentioned.

@klickverbot klickverbot merged commit 73aa70f into from
@klickverbot
Collaborator

@donc: Could you take care of adding any necessary bug reports/documentation notes please? As far as I can tell, the issue you mentioned is not really related to this pull request, and you are our resident IEEE 754 expert.

@don-clugston-sociomantic

@klickverbot: The issue I mentioned is directly relevant. There are two lines of changes to the documentation in this pull request, which are wrong. Basically it's documenting a bug. I have created a bug report:
http://d.puremagic.com/issues/show_bug.cgi?id=9813

@denis-sh

@don-clugston-sociomantic, thanks for the issue. What about documentation improvement?

@don-clugston-sociomantic

The three lines I commented on, should simply be removed. AFAIK there is nothing in the spec which refers to implementation bugs.

@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2013
  1. @denis-sh
  2. @denis-sh

    Fix `std.math.FloatingPointControl` two issues.

    denis-sh authored
    1. Do not call `setControlState` in destructor if not initialized (previously results in enabling all exceptions).
    2. Call `initialize` on setting rounding mode (previously not called).
  3. @denis-sh
This page is out of date. Refresh to see the latest.
Showing with 50 additions and 19 deletions.
  1. +50 −19 std/math.d
View
69 std/math.d
@@ -2778,23 +2778,36 @@ void resetIeeeFlags() { IeeeFlags.resetIeeeFlags(); }
Example:
- ----
- {
+----
+{
+ FloatingPointControl fpctrl;
+
// Enable hardware exceptions for division by zero, overflow to infinity,
// invalid operations, and uninitialized floating-point variables.
-
- FloatingPointControl fpctrl;
fpctrl.enableExceptions(FloatingPointControl.severeExceptions);
- double y = x*3.0; // will generate a hardware exception, if x is uninitialized.
- //
+ // This will generate a hardware exception, if x is a
+ // default-initialized floating point variable:
+ real x; // Add `= 0` or even `= real.nan` to not throw the exception.

Yes, that it what it does, but it's a bug -- it's not supposed to generate an exception.
Actually whether it throws an exception or not is different depending on whether you are on an AMD or Intel processor.

If I don't miss something it is a documented behavior. But also I still don't understand how exactly it behaves from you comment. Could you please clarify the documentation (e.g. defining platform depending behavior)?

real x;
should not generate an exception. An exception should only be generated if x is actually used. But the compiler is incorrectly inserting a load instruction when it initializes the variable. This is a compiler bug, and it's my fault, I've never got around to fixing it. It makes the feature practically useless at the moment. The funny thing is that Intel processors do not generate exceptions when they load 80-bit signalling NaNs, but AMD processors do, so the bug doesn't always have an effect. (I don't think the difference between the processors is intentional, I think AMD just didn't read Intel's docs carefully enough). Both processors will generate an exception as soon as you do an operation on a signalling NaN.

Being an Intel user I didn't know about such AMD behavior. The comment "This will generate..." was about the second line (as for me it was "obviously" a source of exception).

Could you add your comment to FloatingPointControl's documentation? With addition of "Defining platform depending behavior" section and "Bugs" section? It will really help a lot for people to understand what really happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ real y = x * 3.0;
+
+ // The exception is only thrown for default-uninitialized NaN-s.
+ // NaN-s with other payload are valid:
+ real z = y * real.nan; // ok
+
+ // Changing the rounding mode:
fpctrl.rounding = FloatingPointControl.roundUp;
+ assert(rint(1.1) == 2);
- // The hardware exceptions will be disabled when leaving this scope.
+ // The set hardware exceptions will be disabled when leaving this scope.
// The original rounding mode will also be restored.
- }
+}
- ----
+// Ensure previous values are returned:
+assert(!FloatingPointControl.enabledExceptions);
+assert(FloatingPointControl.rounding == FloatingPointControl.roundToNearest);
+assert(rint(1.1) == 1);
+----
*/
struct FloatingPointControl
@@ -2852,8 +2865,8 @@ public:
//// Change the floating-point hardware rounding mode
@property void rounding(RoundingMode newMode)
{
- ushort old = getControlState();
- setControlState((old & ~ROUNDING_MASK) | (newMode & ROUNDING_MASK));
+ initialize();
+ setControlState((getControlState() & ~ROUNDING_MASK) | (newMode & ROUNDING_MASK));
}
/// Return the exceptions which are currently enabled (unmasked)
@@ -2872,7 +2885,8 @@ public:
~this()
{
clearExceptions();
- setControlState(savedState);
+ if (initialized)
+ setControlState(savedState);
}
private:
@@ -2963,20 +2977,37 @@ private:
unittest
{
- {
+ void ensureDefaults()
+ {
+ assert(FloatingPointControl.rounding
+ == FloatingPointControl.roundToNearest);
+ assert(FloatingPointControl.enabledExceptions == 0);
+ }
+
+ {
+ FloatingPointControl ctrl;
+ }
+ ensureDefaults();
+
+ {
+ FloatingPointControl ctrl;
+ ctrl.rounding = FloatingPointControl.roundDown;
+ assert(FloatingPointControl.rounding == FloatingPointControl.roundDown);
+ }
+ ensureDefaults();
+
+ {
FloatingPointControl ctrl;
ctrl.enableExceptions(FloatingPointControl.divByZeroException
- | FloatingPointControl.overflowException);
+ | FloatingPointControl.overflowException);
assert(ctrl.enabledExceptions ==
- (FloatingPointControl.divByZeroException
- | FloatingPointControl.overflowException));
+ (FloatingPointControl.divByZeroException
+ | FloatingPointControl.overflowException));
ctrl.rounding = FloatingPointControl.roundUp;
assert(FloatingPointControl.rounding == FloatingPointControl.roundUp);
}
- assert(FloatingPointControl.rounding
- == FloatingPointControl.roundToNearest);
- assert(FloatingPointControl.enabledExceptions ==0);
+ ensureDefaults();
}
Something went wrong with that request. Please try again.