Add a D implementation for std.math.ilogb #3186

JohanEngelen · 2015-04-12T21:53:03Z

The reasons for this PR:

Gets rid of assembly code that is DMD-specific and does not work for LDC. Special case for Windows CRT is no longer needed.
Avoiding the x87 FPU means this implementation is faster than the assembly code.
Not calling C-runtime: ilogb can be inlined and the compiler can reason about it.
Templated so it runs faster for floats and doubles

Some quick benchmarking showed that it is indeed quite a bit faster, especially (not surprising) for float and double. But the first reason is my main motivation.

I do not know how to do the casts to enable CTFE.
I added a few extra unittests to increase code coverage.

(edit: removed wrong comment about ARM)

Orvid · 2015-04-13T03:27:41Z

std/math.d

@@ -2519,6 +2519,21 @@ unittest
    }
 }

+private import core.bitop : bsr;
+static if (2*size_t.sizeof == ulong.sizeof)


Are you really sure this is actually needed?

JohanEngelen · 2015-04-13T15:57:30Z

@Orvid The code does need a bsr that works for 64-bit numbers on all platforms (32 and 64 bit), which druntime does not provide. But you are right: it is not needed outside ilogb. To fix further function name resolution problems, I've renamed the 64-bit bsr to bsr_ulong.

I've rebased the changes into one commit.

Safety0ff · 2015-04-13T18:13:17Z

std/math.d

-            ret                         ;
+            size_t* uix = cast(size_t*)&x;
+            size_t msb = uix[MANTISSA_MSB];
+            size_t lsb = uix[MANTISSA_LSB];


Nitpick: I would prefer

uint msb = x >> 32; if (msb) return bsr(msb) + 32; else return bsr(cast(uint)x);

Over pointer casting.

Thanks.
That makes it CTFE-able I think?

@Safety0ff That also gets rid of Endianness-noise. I've amended the PR.

JohanEngelen · 2015-04-20T19:32:00Z

The testbot failure is (as far as I can see) completely unrelated to this PR.

…3186

9il · 2015-05-01T20:51:32Z

std/math.d

-    version (Win64_DMD_InlineAsm)
+    import core.bitop : bsr;
+    // Provide a bsr implementation for ulong in 32-bit mode
+    int bsr_ulong(ulong x) @trusted pure nothrow @nogc


Could you move this function outside from ilogb?

static import core.bitop.bsr; ... static if(size_t.sizeof == 4) { int bsr_ulong(ulong x) @trusted pure nothrow @nogc { ... } } else private alias bsr_ulong = core.bitop.bsr;

The reason is to reduce template bloat.

There's also a bsr64 implementation in std.numeric - it's private, however... https://github.com/D-Programming-Language/phobos/blob/965744f7d010082a8e2a68c3ec45734f5d845aff/std/numeric.d#L85

... it is outdated. Looks like we need package bsr64 in std.math.

@ibuclaw How sure are you that the bsr64 in std.numeric works for both little endian and big endian? Using a union instead of bitshifts makes a large assumption about memory layout that I am not sure is justified.

ibuclaw · 2015-05-01T21:43:53Z

std/math.d

+    static if (F.realFormat == RealFormat.ieeeExtended)
+    {
+        if (ex)
+        {   // If exponent is non-zero


I do believe the coding style of Phobos discourages comments nestled in on the same line as opening braces. Please move this and others on the next line.

JohanEngelen · 2015-05-03T11:07:25Z

@ibuclaw and @9il Thanks for review, I've improved the changes, rebased, and are waiting for Travis results.

ibuclaw · 2015-05-03T13:32:12Z

std/math.d

+        ushort[T.sizeof/2] vu;
+        uint[T.sizeof/4] vui;
+        static if(T.sizeof >= 8)
+            long[T.sizeof/8] vl;


Unfortunately, T.sizeof could be 12. So a more complicated logic other than dividing by 8 is required. What I may do in future is move this to be a part of floatTraits.

In the case of T.sizeof == 12 or 10, I was assuming this would truncate to long[1], which is fine? (the code knows about this)
edit: because real.sizeof==10, this case is tested already and seems to work :)

edit2: OK, I did not think this through thoroughly... there is the potential for problems with memory layout of union members smaller than the union size. The original code (parts from frexp) may have a similar problem with bigendian/littleendian with 80bit reals. However, in case of trouble, the unittests will fail.

JohanEngelen · 2015-05-05T18:16:56Z

rebased for cleaner history.

JohanEngelen · 2015-07-20T21:23:06Z

removed trailing whitespace, and rebased

9il · 2015-07-20T23:06:11Z

Take a look at #3499 .

JohanEngelen · 2015-07-22T20:54:12Z

@9il Do you mean that this PR should implement the same as #3499 ? That sounds backwards. I think this PR should be merged (it's been sitting here for 2+ months). Then when #3499 settles it can apply the same changes from frexp to ilogb.

9il · 2015-07-22T20:57:07Z

No, only FYI. It can be merged with current implementation, thought.

JohanEngelen · 2015-07-22T21:07:10Z

OK, sorry for my half-angry comment... :(
Thanks for the heads up. #3499 is certainly an improvement over the current hack (at least it concentrates the hack to one central point ;)

DmitryOlshansky · 2015-07-28T06:14:16Z

@9il anything to review here or is it good to go?

9il · 2015-07-28T08:08:50Z

LGTM except unittest.
@JohanEngelen Please generalise it with

foreach(F; TypeTuple!(float, double real))
{
...
}

JohanEngelen · 2015-07-28T21:29:20Z

@9il I've tried to generalize the unittests, using a construct found elsewhere in math.d. (I am very new to D) Thanks for the patience.

9il · 2015-07-28T21:54:59Z

Thank you for math PRs :)
Rebase all commits into one. @DmitryOlshansky probably will merge.

9il · 2015-07-28T21:59:23Z

std/math.d

+            tuple( -F.infinity, int.max     ),
+            tuple(  2.0       , 1           ),
+            tuple(  2.0001    , 1           ),
+            tuple(  1.9999    , 0           ),


To fix auto test failure use construction like F(1.9999) instead

other option:

alias T = Tuple!(F, int); T[] vals = [ T(1.9999, 1), .... ];

9il · 2015-07-29T13:52:01Z

@JohanEngelen You can use fixed size arrays to prevent GC allocations.

…oating point types.

JohanEngelen · 2015-07-29T17:42:42Z

@9il @DmitryOlshansky All done now!

DmitryOlshansky · 2015-07-29T19:31:47Z

Going to merge in day or so unless somebody rises an issue with it.
@ibuclaw ?

DmitryOlshansky · 2015-08-01T06:45:39Z

Hm a potential regression with this is that it was callable wirg int by implicit conversion and now it won't match the constraint.

JohanEngelen · 2015-08-01T08:35:37Z

@DmitryOlshansky How is this solved in general in Phobos?

9il · 2015-08-01T08:38:23Z

see also #3014 #3012

DmitryOlshansky · 2015-08-01T10:22:36Z

In this case I'd add an overload for isIntegral that internally just does cast(double) for 32bit nunbers and below and cast(real) for everything else.

On the other hand - log2 is trivially implementable for integers with test and bsr.

9il · 2015-08-01T10:24:36Z

The overload should be mark with deprecated, thought.

DmitryOlshansky · 2015-08-01T14:44:07Z

@9il I'm not so sure - it's quite customary to supply integer literal for FP function expecting implicit conversion. Otherwise we'd get welcome user with hard to understand 'doesn't match overload set'.

9il · 2015-08-01T16:45:45Z

@DmitryOlshansky This is bad practice that can cause out-of-the-way errors in mathematical algorithms. However, if a function can have correct and optimal implementation for integer types (like ilogb), then an integer version of this function can be added to library without deprecation.

DmitryOlshansky · 2015-08-01T17:30:45Z

@9il I know back in the university I've seen plenty of bugs in numerics due to use of int in C programs.

JohanEngelen · 2015-08-01T22:23:02Z

Do you want me to add a unittest explicitly testing for a few (large) integer numbers?

DmitryOlshansky · 2015-08-02T05:49:38Z

Do you want me to add a unittest explicitly testing for a few (large) integer numbers?

Yes, integer version needs some tests to cover it.

ibuclaw · 2015-08-02T05:53:50Z

std/math.d

    assert(ilogb(nextUp(-float.min_normal)) == -127);
-    assert(ilogb(nextUp(-0.0F)) == -149);
-    static if (floatTraits!(real).realFormat == RealFormat.ieeeExtended) {
+    assert(ilogb(nextUp(-float(0.0))) == -149);


JohanEngelen · 2015-08-02T21:48:03Z

@9il @DmitryOlshansky I've added implementations and tests for integer ilogb arguments. What do you think?

DmitryOlshansky · 2015-08-02T22:50:45Z

Lookin' good

DmitryOlshansky · 2015-08-02T22:50:59Z

Auto-merge toggled on

Add a D implementation for std.math.ilogb

Orvid reviewed Apr 13, 2015
View reviewed changes

JohanEngelen force-pushed the new_ilogb branch from 223eaf4 to d3ad70e Compare April 13, 2015 15:55

Safety0ff reviewed Apr 13, 2015
View reviewed changes

JohanEngelen force-pushed the new_ilogb branch 2 times, most recently from af51714 to 55a2a8d Compare April 13, 2015 18:52

JohanEngelen added a commit to JohanEngelen/phobos that referenced this pull request Apr 20, 2015

Fix Win64 compilation by replacing ilogb with a D version. See dlang#…

e912851

…3186

JohanEngelen mentioned this pull request Apr 20, 2015

Fix Win64 compilation by replacing ilogb with a D version ldc-developers/phobos#18

Merged

9il reviewed May 1, 2015
View reviewed changes

9il mentioned this pull request May 1, 2015

std.mathspecial: for 64-bit reals, provide MAXGAMMA, MAXLOG, and MINLOG #3045

Merged

ibuclaw reviewed May 1, 2015
View reviewed changes

JohanEngelen force-pushed the new_ilogb branch from 55a2a8d to 95dc9f1 Compare May 3, 2015 11:05

ibuclaw reviewed May 3, 2015
View reviewed changes

JohanEngelen force-pushed the new_ilogb branch from 17d65f1 to f9de5ea Compare May 5, 2015 18:14

DmitryOlshansky added the math label Jul 15, 2015

JohanEngelen force-pushed the new_ilogb branch from 5274f2f to c6404ab Compare July 20, 2015 21:22

9il reviewed Jul 28, 2015
View reviewed changes

JohanEngelen force-pushed the new_ilogb branch from ca2ba02 to d293305 Compare July 29, 2015 17:08

Add a D implementation for std.math.ilogb, templated for different fl…

98638ab

…oating point types.

JohanEngelen force-pushed the new_ilogb branch from d293305 to 98638ab Compare July 29, 2015 17:10

ibuclaw reviewed Aug 2, 2015
View reviewed changes

Add integer ilogb

321f8d7

DmitryOlshansky added a commit that referenced this pull request Aug 2, 2015

Merge pull request #3186 from JohanEngelen/new_ilogb

14301c6

Add a D implementation for std.math.ilogb

DmitryOlshansky merged commit 14301c6 into dlang:master Aug 2, 2015

JohanEngelen pushed a commit to ldc-developers/phobos that referenced this pull request Nov 9, 2015

Update math.d.ilogb from Phobos master (PR dlang#3186)

45a07e3

Add a D implementation for std.math.ilogb #3186

Add a D implementation for std.math.ilogb #3186

Conversation

JohanEngelen commented Apr 12, 2015

Choose a reason for hiding this comment

JohanEngelen commented Apr 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Apr 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented May 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented May 5, 2015

JohanEngelen commented Jul 20, 2015

9il commented Jul 20, 2015

JohanEngelen commented Jul 22, 2015

9il commented Jul 22, 2015

JohanEngelen commented Jul 22, 2015

DmitryOlshansky commented Jul 28, 2015

9il commented Jul 28, 2015

JohanEngelen commented Jul 28, 2015

9il commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9il commented Jul 29, 2015

JohanEngelen commented Jul 29, 2015

DmitryOlshansky commented Jul 29, 2015

DmitryOlshansky commented Aug 1, 2015

JohanEngelen commented Aug 1, 2015

9il commented Aug 1, 2015

DmitryOlshansky commented Aug 1, 2015

9il commented Aug 1, 2015

DmitryOlshansky commented Aug 1, 2015

9il commented Aug 1, 2015

DmitryOlshansky commented Aug 1, 2015

JohanEngelen commented Aug 1, 2015

DmitryOlshansky commented Aug 2, 2015

Choose a reason for hiding this comment

JohanEngelen commented Aug 2, 2015

DmitryOlshansky commented Aug 2, 2015

DmitryOlshansky commented Aug 2, 2015