fix issue 17243 - std.math.{FloatingPointControl,ieeeFlags} don't work on x86_64 by aG0aep6G · Pull Request #5240 · dlang/phobos

aG0aep6G · 2017-03-04T15:31:39Z

I'm far from being an expert on this stuff. I learned about the different control registers only when investigating this issue here. If someone more experienced wants to take over, be my guest.

I have a test file for unmasked exceptions, but I don't know how to incorporate it into the phobos tests.

A test for roundToNearest is missing, because I'm having a hard time thinking of one.

dlang-bot · 2017-03-04T15:31:40Z

Fix	Bugzilla	Description
✓	17243	std.math.{FloatingPointControl,ieeeFlags} don't work on x86_64

aG0aep6G · 2017-03-04T19:00:26Z

It's my understanding that the issue can also manifest on 32-bit x86s when the compiler is configured to emit SSE instructions (not sure if that can be done with dmd).There doesn't seem to be a version for SSE, so I don't know how to approach that variant of the issue.

The failures of the doc tester and CircleCI don't make sense to me. The doc tester says:

../phobos/std/traits.d(6620): Error: static assert "Type uint does not have an Unsigned counterpart"

And CircleCI says:

out/std_regex_package.d(43): Error: undefined identifier 'popFrontN'

Both seem unrelated.

ghost · 2017-03-05T10:02:23Z

It's my understanding that the issue can also manifest on 32-bit x86s when the compiler is configured to emit SSE instructions (not sure if that can be done with dmd)

On x86, even if for rounding DMD emitts code for the FPU one may use the std.math API and write custom asm SSE code that relies on it, hence both FPU and SSE flags should be set. (IMO, this can be discussed)

aG0aep6G · 2017-03-05T19:19:14Z

On x86, even if for rounding DMD emitts code for the FPU one may use the std.math API and write custom asm SSE code that relies on it, hence both FPU and SSE flags should be set.

I've added a fallback to CPUID for x86.

From my end, this is good to go. I've fixed the test failures and squashed the fixup commits. auto-tester is green (pending testing of the fallback mentioned above).

aG0aep6G · 2017-03-05T19:20:21Z

std/math.d

 }

+version (X86_64) version = StaticallyHaveSSE;
+version (X86) version (OSX) version = StaticallyHaveSSE;


I'm not sure if this is correct. Does OSX require SSE? This is here, because I've noticed that dmd emits SSE code for 32-bit OSX.

It's b/c Apple only ever sold x86_64 CPUs.

aG0aep6G · 2017-03-05T19:21:54Z

std/math.d

            INVALID_MASK   = 0x01
        }
-        // Don't bother about subnormals, they are not supported on most CPUs.
-        //  SUBNORMAL_MASK = 0x02;


Comment seemed bogus. My run-of-the-mill AMD CPU and the auto-tester ones apparently support it.

aG0aep6G · 2017-03-05T19:24:27Z

std/math.d


+     /** A subnormal number was used in a calculation.
+     (example: x = real.min_normal / 2 + 1) */
+     @property bool subnormal() { return (flags & SUBNORMAL_MASK) != 0; }


@andralex: Adding a new method here. As far as I remember, you want to review new names.

I'm cool, please make const and so the others

please make const and so the others

Made this one const. New PR for the others as it's independent from this PR: #5246

aG0aep6G · 2017-03-05T19:26:54Z

std/math.d

-                    fclex;
-                    fldcw   8[RSP];
-                    ret;
-                }


This deleted code looks very clever. Unfortunately, there is no comment on what it achieves. The generic variant below seems to work just fine on Win64, and it's much simpler. So I'm tearing down this fence without knowing why it was put up.

It was just the naked version of the asm block below, no need for that here.

aG0aep6G · 2017-03-05T21:11:02Z

auto-tester is green (pending testing of the fallback mentioned above).

Of course it went red then. Fixed getIeeeFlags and resetIeeeFlags. All green again.

ibuclaw · 2017-03-06T22:39:31Z

std/math.d


+     /** A subnormal number was used in a calculation.
+     (example: x = real.min_normal / 2 + 1) */
+     @property bool subnormal() const { return (flags & SUBNORMAL_MASK) != 0; }


The majority of targets don't have an exception for denormals, I don't think this should be added here.

The majority of targets don't have an exception for denormals, I don't think this should be added here.

As far as I see, IeeeFlags only works on x86 and x86-64 at the moment. Everything else errors out with "not yet supported". x86(-64) are the ones that have this, right? Would it be ok if this was versioned away from the others?

As far as I see, IeeeFlags only works on x86 and x86-64 at the moment. Everything else errors out with "not yet supported"

GDC has code for ARM as well: https://github.com/D-Programming-GDC/GDC/blob/master/libphobos/src/std/math.d#L4372 and LDC probably supports even more targets. The part of the code that has never been upstreamed is mostly GDC extended ASM so it's not useful for other compilers anyway.

We unfortunately don't have any guidelines yet how to deal with architecture specific features (there's a similar problem for cpuid). I guess we could add this function but we should at least make sure to explicitly document it as non-portable.

aG0aep6G · 2017-03-08T13:26:44Z

I've removed the new subnormal method as it's not really related to issue 17243. The method can be discussed and added separately.

I've also squashed everything into one commit. The commit list was becoming messy.

…k on x86_64

dlang-bot added the Severity:Bug Fix label Mar 4, 2017

aG0aep6G changed the base branch from master to stable March 4, 2017 15:31

aG0aep6G force-pushed the FloatingPointControl branch 2 times, most recently from 1884212 to 624b23f Compare March 4, 2017 15:50

aG0aep6G changed the title ~~fix issue 17243 - std.math.FloatingPointControl doesn't work on x86_64~~ fix issue 17243 - std.math.{FloatingPointControl,ieeeFlags} don't work on x86_64 Mar 4, 2017

aG0aep6G force-pushed the FloatingPointControl branch from 9b53aed to 0cc2e2d Compare March 4, 2017 18:45

aG0aep6G force-pushed the FloatingPointControl branch from bb53e70 to 3536c56 Compare March 4, 2017 21:57

aG0aep6G force-pushed the FloatingPointControl branch 2 times, most recently from d1aebd5 to 1bf8837 Compare March 5, 2017 19:10

aG0aep6G commented Mar 5, 2017

View reviewed changes

aG0aep6G force-pushed the FloatingPointControl branch from f7d16c4 to 0ecb8d7 Compare March 5, 2017 20:38

aG0aep6G mentioned this pull request Mar 5, 2017

std.math.IeeeFlags: make getters const #5246

Merged

andralex requested review from WalterBright and ibuclaw March 5, 2017 23:20

ibuclaw reviewed Mar 6, 2017

View reviewed changes

aG0aep6G force-pushed the FloatingPointControl branch from 85eb329 to f173c72 Compare March 8, 2017 13:21

fix issue 17243 - std.math.{FloatingPointControl,ieeeFlags} don't wor…

09679c2

…k on x86_64

aG0aep6G force-pushed the FloatingPointControl branch from f173c72 to 09679c2 Compare March 11, 2017 09:55

MartinNowak approved these changes Mar 14, 2017

View reviewed changes

MartinNowak merged commit 83e1999 into dlang:stable Mar 14, 2017

aG0aep6G deleted the FloatingPointControl branch March 14, 2017 15:53

Uh oh!

Conversation

aG0aep6G commented Mar 4, 2017

Uh oh!

dlang-bot commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aG0aep6G commented Mar 4, 2017

Uh oh!

ghost commented Mar 5, 2017

Uh oh!

aG0aep6G commented Mar 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Mar 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dlang-bot commented Mar 4, 2017 •

edited

Loading

aG0aep6G Mar 5, 2017 •

edited

Loading