std.math: Use core.stdc.fenv for exception mask values by ibuclaw · Pull Request #4272 · dlang/phobos

ibuclaw · 2016-04-30T21:20:59Z

FloatingPointControl rounding control and interrupt masks should be given similar treatment. Where their values all correspond to macros from fpu_control.h in libc (there does exist any binding in druntime).

ibuclaw · 2016-05-01T04:48:22Z

Can someone check the fenv header for win32? Should it be the same as CRuntime_Microsoft or not?

wilzbach · 2016-05-01T05:00:58Z

std/math.d

+        UNDERFLOW_MASK = FE_UNDERFLOW,
+        OVERFLOW_MASK  = FE_OVERFLOW,
+        DIVBYZERO_MASK = FE_DIVBYZERO,
+        INVALID_MASK   = FE_INVALID


Why do we even need those? Wouldn't it be better to directly replace all occurrences?
As far as I can tell the enum is private

One change at a time. ;-)

But yeah they probably should go away.

ibuclaw · 2016-05-01T07:08:48Z

Ah, it is win64 that is failing. Ping @kinke as you added that path. Why does Win64 deviate from hardware?

kinke · 2016-05-01T08:54:25Z

fenv.h from Windows 10 Universal C Runtime (shipped with Visual Studio 2015), no special #ifdefs:

#define FE_INEXACT   _SW_INEXACT       // _EM_INEXACT     0x00000001 inexact (precision)
#define FE_UNDERFLOW _SW_UNDERFLOW     // _EM_UNDERFLOW   0x00000002 underflow
#define FE_OVERFLOW  _SW_OVERFLOW      // _EM_OVERFLOW    0x00000004 overflow
#define FE_DIVBYZERO _SW_ZERODIVIDE    // _EM_ZERODIVIDE  0x00000008 zero divide
#define FE_INVALID   _SW_INVALID       // _EM_INVALID     0x00000010 invalid

Since when does MS not like to go their own way? 😁

So I'd probably keep the fixed constants for X86 to work around this CRuntime_Microsoft special case and use the fenv constants for all other architectures.

ibuclaw · 2016-05-01T09:21:36Z

OK, finally found a reference header:
https://github.com/adityaramesh/concurrency_runtime/blob/c92c41b8ff41657a164c7760fd67b77d84ba766f/VS2013_SP2/include/fenv.h#L39-L45

How does this work? It's the CPU that sets the flags, not the OS. 😕

ibuclaw · 2017-01-15T09:11:37Z

Ping? (I should add a comment here about why windows is a special case)

ibuclaw · 2017-01-20T07:56:10Z

This depends on druntime change getting in first before this - at least for non-x86. This is just moving enum values to their proper locations in the stdc bindings. I am disappointed at Microsoft though for having masks that don't match the hardware.

I should probably replace X86_Any here with just CRuntime_Microsoft.

MetaLang · 2017-08-06T05:23:10Z

@ibuclaw is this the Druntime PR you're referring to? dlang/druntime#1554

ibuclaw · 2017-08-06T13:29:28Z

@MetaLang - Yes... I should update the druntie PR to include the vagary of architectures we've added support for since then.

(By the way, isn't the link in my original post working?)

MetaLang · 2017-08-06T15:07:19Z

@ibuclaw yeah it does. Sorry, I was half asleep when I wrote that.

wilzbach · 2017-08-06T15:13:29Z

I was half asleep when I wrote that.

Don't worry about this. Many people including myself have the same problem of finding time and often the night is the only available spot :/

PetarKirov

LGTM, apart from my question about removing the version (X86_Any) definitions altogether and using only the aliases like INEXACT_MASK = FE_INEXACT which forward to druntime.

With that fixed, this should be good to go.

PetarKirov · 2017-08-07T09:18:40Z

std/math.d

+    // SPARC FSR is a 32bit register (64 bits for SPARC 7 & 8, but high bits are uninteresting).
    uint flags;
+
    version (X86_Any)


Can we remove these constants too, as they should now be pulled from Druntime?

See earlier conversation on Windows being annoyingly different, which causes testsuite failures.

Can you add some short explanation as a comment, so that people won't be tempted to "improve" the code?

Proposal:

// Microsoft uses hardware-incompatible custom constants in fenv.h (core.stdc.fenv).

Also, there's my suggestion of using version CRuntime_Microsoft.

[Microsoft is gonna try again with Windows on ARM, so CRuntime_Microsoft may not always imply x86(_64).]

That's fine, because it will probably use the same constants for both x86 and ARM. :-)

A daring assumption. ;)

Can we move CRuntime_Microsoft to druntime? And if necessary add version (X86_Any) and version (ARM) inside.

It already is in druntime, but as stated before, the Microsoft headers don't appear to match the hardware. And so the duplication is an unfortunate evil.

Maybe there's a rational explanation like Microsoft get and set feflags differently, or mask them somehow to translate to the "correct" values. Correct being in quotation marks.

I'll get round to actually rebasing this tonight, rather than talking about it in passing.

ibuclaw · 2017-10-08T14:51:29Z

I've just hit this again whilst getting mips CI working with gdc. Can we merge this, glossing over the fac tthat CRuntime_Microsoft is different? (Shame on you Microsoft).

dlang-bot · 2017-10-08T14:57:10Z

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

ibuclaw · 2017-10-08T15:04:42Z

Rebased against master.

PetarKirov · 2017-10-08T16:04:09Z

std/math.d

    {
-        // SPARC FSR is a 32bit register
-             //(64 bits for Sparc 7 & 8, but high 32 bits are uninteresting).
+        import core.stdc.fenv;


Circle CI fails:

std/math.d(4682:16)[warn]: Local imports should specify the symbols being imported to avoid hiding local symbols.

I'll just statically import it. There are a few other places where std.math overlaps with core.stdc.fenv.

wilzbach reviewed May 1, 2016
View reviewed changes

ibuclaw force-pushed the ieeefenvflags branch from 14e758b to 0e210ce Compare May 1, 2016 09:31

9il added the math label May 1, 2016

JackStouffer mentioned this pull request Jan 19, 2017

Update zlib to the 1.2.10 release #5031

Merged

wilzbach added the Merge:Blocked label Mar 5, 2017

dlang-bot added the Merge:stalled label Jun 22, 2017

PetarKirov removed Merge:Blocked Merge:stalled labels Aug 7, 2017

PetarKirov approved these changes Aug 7, 2017

View reviewed changes

ibuclaw added the Review:Blocking Other Work label Oct 8, 2017

ibuclaw force-pushed the ieeefenvflags branch from 0e210ce to 013819d Compare October 8, 2017 14:57

ibuclaw requested a review from dnadlinger as a code owner October 8, 2017 14:57

ibuclaw force-pushed the ieeefenvflags branch 2 times, most recently from ac2fc30 to e95ec1b Compare October 8, 2017 15:03

PetarKirov reviewed Oct 8, 2017

View reviewed changes

std.math: Use core.stdc.fenv for exception mask values

74f687c

ibuclaw force-pushed the ieeefenvflags branch from e95ec1b to 74f687c Compare October 8, 2017 20:14

ibuclaw force-pushed the ieeefenvflags branch from 489a788 to 74f687c Compare October 8, 2017 21:38

PetarKirov approved these changes Oct 8, 2017

View reviewed changes

PetarKirov added the Merge:auto-merge label Oct 8, 2017

dlang-bot merged commit e974a88 into dlang:master Oct 8, 2017

ibuclaw deleted the ieeefenvflags branch October 8, 2017 22:53

ibuclaw mentioned this pull request Oct 8, 2017

Small clean-ups of architecture-specific code in FloatingPointControl #5769

Merged

Uh oh!

Conversation

ibuclaw commented Apr 30, 2016

Uh oh!

ibuclaw commented May 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented May 1, 2016

Uh oh!

kinke commented May 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented May 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Jan 15, 2017

Uh oh!

ibuclaw commented Jan 20, 2017

Uh oh!

MetaLang commented Aug 6, 2017

Uh oh!

ibuclaw commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetaLang commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Aug 6, 2017

Uh oh!

PetarKirov left a comment • 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!

PetarKirov Aug 7, 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!

kinke Aug 7, 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!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Oct 8, 2017

Uh oh!

dlang-bot commented Oct 8, 2017

Bugzilla references

Uh oh!

ibuclaw commented Oct 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kinke commented May 1, 2016 •

edited

Loading

ibuclaw commented May 1, 2016 •

edited

Loading

ibuclaw commented Aug 6, 2017 •

edited

Loading

MetaLang commented Aug 6, 2017 •

edited

Loading

PetarKirov left a comment •

edited

Loading

PetarKirov Aug 7, 2017 •

edited

Loading

kinke Aug 7, 2017 •

edited

Loading