Implement phase 1 of expanded Floating
ClosedPublic

Authored by dolio on Dec 12 2015, 2:23 PM.

Details

Summary
  • This part of the proposal is to add log1p, expm1, log1pexp and log1mexp to the Floating class, and export the full Floating class from Numeric

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dolio updated this revision to Diff 5643.Dec 12 2015, 2:23 PM
dolio retitled this revision from to Implement most of phase 1 of expanded Floating.
dolio updated this object.
dolio edited the test plan for this revision. (Show Details)
dolio updated the Trac tickets for this revision.
dolio updated this revision to Diff 5644.Dec 12 2015, 2:29 PM
dolio edited edge metadata.
  • Document Floating expansion in changelog
dolio added a comment.Dec 12 2015, 2:32 PM

Design is here, I'm just implementing it:

https://prime.haskell.org/wiki/Libraries/Proposals/ExpandFloating#Phase1GHC8.0

hvr says he knows who can do the libm portion of this, so I think that's the next step. I'm not sure if that would be included in this differential, or a separate one.

austin accepted this revision.Dec 12 2015, 3:14 PM
austin edited edge metadata.
In D1605#47576, @dolio wrote:

Design is here, I'm just implementing it:

https://prime.haskell.org/wiki/Libraries/Proposals/ExpandFloating#Phase1GHC8.0

hvr says he knows who can do the libm portion of this, so I think that's the next step. I'm not sure if that would be included in this differential, or a separate one.

That would be separate. This looks great to me.

This revision is now accepted and ready to land.Dec 12 2015, 3:14 PM
hvr accepted this revision.Dec 12 2015, 3:53 PM
hvr edited edge metadata.

lgtm

@dolio btw can you think of any sensible unit-tests for the usual suspects Complex/Double/Float?

ekmett accepted this revision.Dec 12 2015, 4:06 PM
ekmett added a reviewer: ekmett.
ekmett added a subscriber: ekmett.

LGTM

hvr requested changes to this revision.Dec 12 2015, 4:21 PM
hvr edited edge metadata.

Btw, as time is tight for GHC 8, we may just want to do a simple FFI call to libm's log1p/expm1 (as this is more about numerical precision than performance). Otoh, there doesn't seem to be log1pexp/log1mexp in libm.

The wiki page suggests more accurate versions of log1mexp/log1mexp for that case. @dolio could you integrate that?

This revision now requires changes to proceed.Dec 12 2015, 4:21 PM
dolio updated this revision to Diff 5647.Dec 12 2015, 6:05 PM
dolio edited edge metadata.
  • Implement libm-based Floating ops
dolio added a comment.Dec 12 2015, 6:09 PM

I've added Float and Double implementations based on libm foreign imports.

I don't think I know enough about numeric stuff to know what kind of unit tests would be appropriate. Obviously stuff like log1p x == log (1 + x) is out, because the whole point is that log1p might give a different, better answer.

dolio retitled this revision from Implement most of phase 1 of expanded Floating to Implement phase 1 of expanded Floating.Dec 12 2015, 6:09 PM
dolio updated this object.
dolio edited edge metadata.

Do you know if this builds on Windows? I ask since Windows users have been unable to compile logfloat for years due to libm failing to link correctly on Windows. Last time I checked, this was still the case on GHC HEAD, but that might have changed since then.

dolio added a comment.Dec 12 2015, 8:30 PM

I don't. Making sure things built on Windows was also supposed to be someone else's job. I can't verify it myself.

Well, I'm not sure how much time is left until the 8.0 release, but if we can wait until Monday I can build this on my Windows machine at work.

hvr added a comment.Dec 13 2015, 7:36 AM

Well, I'm not sure how much time is left until the 8.0 release, but if we can wait until Monday I can build this on my Windows machine at work.

We'll surely have time till monday :)

This OS called "Windows" seems quite odd in terms of standards compliance... ;-)

hvr added a comment.EditedDec 13 2015, 7:38 AM

@bgamari could you trigger a win32 build of this patch (before monday)? I'm just curious to know asap if there's problems

bgamari accepted this revision.Dec 14 2015, 7:29 AM
bgamari edited edge metadata.

I'm glad to see these incorporated into the standard library. As I point out, however, we need some documentation on the added members.

Otherwise this looks good to me (pending testing on Windows).

libraries/base/GHC/Float.hs
64

I think Haddocks on these would be worthwhile. Yes, they are fairly well-known to those well-versed in numerics but we shouldn't let lack of documentation to be a barrier for those who are not.

RyanGlScott added a subscriber: Phyx.EditedDec 14 2015, 11:58 AM

I just tried this on Windows 64-bit, and unfortunately, my worst fears were confirmed. Compiling modules via ghc works fine, but things fails to link in GHCi:

$ .\inplace\bin\ghc-stage2.exe --interactive
GHCi, version 7.11.20151214: http://www.haskell.org/ghc/  :? for help
ghc-stage2.exe: C:\Users\ryanscot\Documents\Software\ghc\libraries\base\dist-install\build\HSbase-4.9.0.0.o: unknown symbol `expm1f'
ghc-stage2.exe: unable to load package `base-4.9.0.0'

I'm cc'ing @Phyx, who is currently exploring a solution to this problem.

hvr added a comment.Dec 14 2015, 2:37 PM

I just tried this on Windows 64-bit, and unfortunately, my worst fears were confirmed. Compiling modules via ghc works fine, but things fails to link in GHCi:

It seems that the following workaround helps here:

diff --git a/rts/RtsSymbols.c b/rts/RtsSymbols.c
index 8413d31..4b0a1d5 100644
--- a/rts/RtsSymbols.c
+++ b/rts/RtsSymbols.c
@@ -157,6 +157,10 @@
       SymI_HasProto(erfc)                                \
       SymI_HasProto(erff)                                \
       SymI_HasProto(erfcf)                               \
+      SymI_HasProto(expm1)                               \
+      SymI_HasProto(expm1f)                              \
+      SymI_HasProto(log1p)                               \
+      SymI_HasProto(log1pf)                              \
       SymI_HasProto(memcpy)                              \
       SymI_HasProto(rts_InstallConsoleEvent)             \
       SymI_HasProto(rts_ConsoleHandlerDone)              \

Can you try if this helps?

Phew, luckily that patch fixes it. (And as an added bonus, logfloat now works correctly in GHCi! Yay!)

hvr added a comment.Dec 14 2015, 3:54 PM

Phew, luckily that patch fixes it. (And as an added bonus, logfloat now works correctly in GHCi! Yay!)

Now the only thing missing is an explanation why this actually works... :-)

Phyx added a comment.Dec 14 2015, 4:21 PM
In D1605#48034, @hvr wrote:

Phew, luckily that patch fixes it. (And as an added bonus, logfloat now works correctly in GHCi! Yay!)

Now the only thing missing is an explanation why this actually works... :-)

This is easily explained. From what I understand, MinGW links against msvcrt.dll which was released in 1998 and doesn't have C99 support of course.
For C99 compatibility they implemented a compatibility library libmingwex.a which can be linked to for symbols you would usually find in libm.

Now libm itself is just a wrapper, a dummy to satisfy linking as the real symbols are in msvcrt.dll. Your missing symbols are all C99 so msvcrt.dll won't have them.
The usual way around this is to link in either msvcr1xx.dll (where xx is 00, 10 or 20 depending on the MSVC runtime version and level of C99 required) or libmingwex.a.
So linking against msvcr120.dll should have solved this as well.

C:\Program Files (x86)\Microsoft Visual Studio 12.0>dumpbin /exports "c:\Windows\System32\msvcr120.dll" | find "expm"
       1567  61E 0008D8F8 expm1 = expm1
       1568  61F 0008DA18 expm1f = expm1f
       1569  620 0008DAF0 expm1l = expm1l

C:\Program Files (x86)\Microsoft Visual Studio 12.0>dumpbin /exports "c:\Windows\System32\msvcr120.dll" | find "log1"
       1515  5EA 00087F54 clog10 = clog10
       1516  5EB 00087FA8 clog10f = clog10f
       1517  5EC 00087FC8 clog10l = clog10l
       1688  697 00092E30 log10 = log10
       1689  698 000933F0 log10f = log10f
       1690  699 000938D8 log1p = log1p
       1691  69A 00093998 log1pf = log1pf
       1692  69B 00093A54 log1pl = log1pl

This is installed by the msvcrt redistributables so it may not be available on all systems by default.

The second way is to link against libmingwex.a which currently won't work because the rts re-exports some symbols from libmingwex.a.
These extra symbols are required for it to compile some libraries like base without any extra linker instructions, but they prevent linking to the full libmingwex
as the introduce duplicate symbol errors.

This is why you can re-export those symbols from the rts and have it work. This is a moving target of course, and we're only fixing a small subset of programs
like this every time. I'm working on a more permanent solution as part of Trac #11223

dolio updated this revision to Diff 5810.Dec 17 2015, 1:32 PM
dolio edited edge metadata.
  • Document new floating point functions.
dolio added a comment.Dec 17 2015, 1:37 PM

So, I've added documentation for the new methods. Unfortunately, I can't see what it looks like, because there appears to be a haddock bug in head that makes classes not have lists of the methods in them, and thus the documentation for those methods doesn't show up.

What should be done with respect to Windows? Should I add those lines to RtsSymbols.c?

In D1605#49032, @dolio wrote:

So, I've added documentation for the new methods. Unfortunately, I can't see what it looks like, because there appears to be a haddock bug in head that makes classes not have lists of the methods in them, and thus the documentation for those methods doesn't show up.

One thing that stands out is that I think bulleted list items have to be surrounded by empty lines. So do this:

-- Examples:
--
-- * if @x@ is a large negative number, @'log' (1 + 'exp' x)@ will be
--   imprecise for the reasons given in 'log1p'.
--
-- * if @'exp' x@ is close to @-1@, @'log' (1 + 'exp' x)@ will be
--   imprecise for the reasons given in 'expm1'.

rather than

-- Examples:
-- * if @x@ is a large negative number, @'log' (1 + 'exp' x)@ will be
--   imprecise for the reasons given in 'log1p'.
-- * if @'exp' x@ is close to @-1@, @'log' (1 + 'exp' x)@ will be
--   imprecise for the reasons given in 'expm1'.

What should be done with respect to Windows? Should I add those lines to RtsSymbols.c?

Yes. @hvr's patch was precisely what was needed to fix the build for me on Windows.

austin requested changes to this revision.Dec 17 2015, 3:51 PM
austin edited edge metadata.

Requesting changes for the Windows fixes.

This revision now requires changes to proceed.Dec 17 2015, 3:51 PM
dolio updated this revision to Diff 5842.Dec 18 2015, 9:11 PM
dolio edited edge metadata.
  • Fix haddock formatting for log1p/mexp
  • Fix building Floating expansion on Windows
dolio added a comment.Dec 18 2015, 9:13 PM

Windows fixes are now in.

I also tested the haddock and made some fixes. It still won't show up with these patches until haddock is fixed, of course.

hvr requested changes to this revision.Dec 19 2015, 2:16 AM
hvr edited edge metadata.

generally looks good to me, I just want us to ponder about some of the INLINEs

libraries/base/Data/Complex.hs
199–211

do we really want to force INLINE here for such a function-body?

libraries/base/GHC/Float.hs
363–371

is a forced INLINE sensible here?

484–492

is a forced INLINE sensible here?

This revision now requires changes to proceed.Dec 19 2015, 2:16 AM
hvr added a comment.Dec 19 2015, 2:18 AM

...and @since annotations! :-)

libraries/base/GHC/Float.hs
66

please add @since 4.9.0.0 here

70

please add @since 4.9.0.0 here

82

please add @since 4.9.0.0 here

94

please add @since 4.9.0.0 here

dolio updated this revision to Diff 5859.Dec 19 2015, 4:24 PM
dolio edited edge metadata.
  • Add @since for floating point expansion.
This revision was automatically updated to reflect the committed changes.