std.math: Add IEEE 128-bit implementation of lrint(). by redstar · Pull Request #4036 · dlang/phobos

redstar · 2016-03-02T08:19:28Z

This is one of the missing functions for 128-bit reals.

tsbockman · 2016-03-09T22:29:20Z

std/math.d

+
+            // Find the exponent and sign
+            ulong msb = vl[MANTISSA_MSB];
+            ulong lsb = vl[MANTISSA_LSB];


I realize you're just copying the names used above in the ieeeDouble section, but these really should be named something else like hi and lo.

msb generally stands for "Most Significant Byte" (or Bit), and lsb for "Least Significant Byte" (or Bit). Using that acronym for 64-bit values (quad words, I think, in Intel terminology?) is confusing.

tsbockman · 2016-03-12T18:47:28Z

std/math.d

+            ulong hi = vl[MANTISSA_MSB];
+            ulong lo = vl[MANTISSA_LSB];
+            int exp = cast(int)((hi >> EXPSHIFT_ULONG) & F.EXPMASK) - F.EXPBIAS;
+            int sign = cast(int)(lo >> 63);


This should be:

ulong hi = vl[MANTISSA_MSB]; int exp = cast(int)((hi >> EXPSHIFT_ULONG) & F.EXPMASK) - F.EXPBIAS; int sign = cast(int)(hi >> 63);

The sign bit is in the hi part, which in turn means that you only actually need the lo part one place:

else { const lo = vl[MANTISSA_LSB]; result = (cast(long) hi << (exp - EXPSHIFT_ULONG)) | (lo >> (MANTISSE - exp)); }

Good point.

This is one of the missing functions for 128-bit reals.

tsbockman · 2016-03-12T19:36:21Z

std/math.d

+
+                if (exp < 0)
+                    result = 0;
+                else if (exp <= 48)


How about else if (exp <= EXPSHIFT_ULONG) ?

tsbockman · 2016-03-12T19:39:19Z

This PR seems OK to me now, pending @9il 's approval.

9il · 2016-03-12T19:41:15Z

I would like @redstar to reorganize this function to template (we should do it for all std.math).

tsbockman · 2016-03-12T19:45:21Z

@9il I think we should plan on doing a clean-sheet std.math2. There are so many different things about the current module that could be improved, if we weren't bound by backwards compatibility.

Either way though, that can be done in a separate PR, yes?

9il · 2016-03-12T19:49:18Z

@9il I think we should plan on doing a clean-sheet std.math2. There are so many different things about the current module that could be improved, if we weren't bound by backwards compatibility.

There is no significant problems with backwards compatibility for math functions, std.math would be upgraded step-by-step.

Either way though, that can be done in a separate PR, yes?

I would like to see the template version in this PR because lrint can be generalized with FloatTraits. FloatTraits can be extended itself.

9il · 2016-03-12T19:55:44Z

32bit and 64bit should be converted to 64bit version
All other versions should have the same algorithm (except constants) like for extended.

tsbockman · 2016-03-12T19:56:24Z

@9il

FloatTraits can be replaced itself.

Would it be worthwhile for me to try and turn this proof-of-concept I did a while ago into a real pull request? #3598 (comment)

I think part of the problem with floatTraits, is that it is too low-level. A wrapper like the one I sketched out in the linked comment would be much easier to use correctly. It should perform basically the same, with inlining.

9il · 2016-03-12T19:59:21Z

FloatTraits can be replaced itself.
Would it be worthwhile for me to try and turn this proof-of-concept I did a while ago into a real pull request? #3598 (comment)

I think part of the problem with floatTraits, is that it is too low-level. A wrapper like the one I sketched out in the linked comment would be much easier to use correctly. It should perform basically the same, with inlining.

This should be extended instead of replaced. My bad.

Current FloatTraits looks OK for me, but if you have good ideas this would be interesting.

tsbockman · 2016-03-12T20:00:36Z

@9il

Current FloatTraits looks OK for me, but if you have good ideas this would be interesting.

Check out FloatParts in the comment I linked.

9il · 2016-03-12T20:06:57Z

@tsbockman Yes, we should have a union based version, because current pointer based algorithms are not CTFE-able. Anyway we need tests and disassembled code to see the performance issues.

9il · 2016-03-12T20:07:25Z

But this is big work

tsbockman · 2016-03-12T20:09:28Z

@9il

But this is big work

Indeed. I think it's time to just bite the bullet and do it, though, as otherwise we'll waste a lot of time refactoring std.math in ways that we aren't going to actually keep for very long.

9il · 2016-03-12T20:19:26Z

Indeed. I think it's time to just bite the bullet and do it, though, as otherwise we'll waste a lot of time refactoring std.math in ways that we aren't going to actually keep for very long.
Hide all checks

In the same time you may port only few functions, so we can make a tests and review.

tsbockman · 2016-03-12T20:36:06Z

In the same time you may port only few functions, so we can make a tests and review.

Yeah I wouldn't try to do everything in one go. The first step is just to get the prerequisites, like FloatParts, working.

ibuclaw · 2016-03-13T10:12:00Z

std/math.d

+            ulong* vl = cast(ulong*)(&x);
+
+            enum EXPSHIFT_ULONG = 48;
+            enum MANTISSE = real.mant_dig - 1;


Are these strictly necessary? I don't think anyone would mind if you just use the numbers directly in the implementation, as they are very specific to this function.

The MANTISSE should always be 112, no?

ibuclaw · 2016-03-13T10:21:58Z

Logic of the 128-bit implementation looks sound to me. I would expect this to pass our current tests on lrint.

DmitryOlshansky · 2016-04-03T08:51:39Z

So anything holding this up? @ibuclaw @9il

wilzbach · 2016-04-17T22:33:28Z

Ping @redstar - do you have time to "templatize" ?

ibuclaw · 2016-04-23T07:43:13Z

So anything holding this up?

Waiting for response to my comments would be one.

wilzbach · 2016-04-27T05:33:02Z

std/math.d

+            {
+                // It is left implementation defined when the number is too large
+                // to fit in a 64bit long.
+                return cast(long) x;


sorry for the dumb question, but isn't there a chance that if we (will) have 128-bit floats, we might see 128-bit integers too?

128 bit floats are reality on Power64 I think

@wilzbach Yep, @redstar himself added support for cent/ucent in various places in Phobos and DMDFE.

so this function should return cent then?

No, the changes so far just moved errors relating to cent out of the parser and into the semantic pass.

@wilzbach

so this function should return cent then?

No - it should return long for now, to be compatible with algorithms that depend on the existing hard-coded return type.

Long-term, all these overly-similar rounding functions - round(), rint(), lrint(), and nearbyint() - could be replaced by a single template function. One of the template parameters can be the return type, allowing round!long() to directly replace lrint(). round!cent() would be supported as well.

JackStouffer · 2017-01-27T04:19:32Z

Eight months of inactivity from the author. Closing.

@redstar Please reopen if you decide to come back to this.

redstar mentioned this pull request Mar 9, 2016

gamma function: add constants for 128-bit reals. #4047

Closed

tsbockman reviewed Mar 9, 2016
View reviewed changes

redstar force-pushed the lrint branch 2 times, most recently from 094724c to 3ea49b2 Compare March 12, 2016 17:27

tsbockman reviewed Mar 12, 2016
View reviewed changes

std.math: Add IEEE 128-bit implementation of lrint().

89b3267

This is one of the missing functions for 128-bit reals.

redstar force-pushed the lrint branch from 3ea49b2 to 89b3267 Compare March 12, 2016 19:33

tsbockman reviewed Mar 12, 2016
View reviewed changes

9il added the math label Mar 12, 2016

ibuclaw reviewed Mar 13, 2016
View reviewed changes

wilzbach reviewed Apr 27, 2016
View reviewed changes

redstar mentioned this pull request May 25, 2016

Incomplete IEEE 128-bit FP (e.g., aarch64) support in Phobos ldc-developers/ldc#1516

Closed

JackStouffer closed this Jan 27, 2017

Uh oh!

Conversation

redstar commented Mar 2, 2016

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!

tsbockman commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

tsbockman commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

tsbockman commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

tsbockman commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

tsbockman commented Mar 12, 2016

Uh oh!

9il commented Mar 12, 2016

Uh oh!

tsbockman commented Mar 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Mar 13, 2016

Uh oh!

DmitryOlshansky commented Apr 3, 2016

Uh oh!

wilzbach commented Apr 17, 2016

Uh oh!

ibuclaw commented Apr 23, 2016

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!

JackStouffer commented Jan 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants