Page MenuHomePhabricator

renenkel (Robert Enenkel)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 9 2018, 8:33 AM (136 w, 5 d)

Recent Activity

Jan 9 2019

renenkel accepted D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

Amy ran Masoud's tests, so I approve.

Jan 9 2019, 1:31 PM
renenkel accepted D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .
Jan 9 2019, 1:30 PM
renenkel added a comment to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

I'm okay with the style of the code now. Did you run Masoud's test program after making your changes? If not, you need to do that.

Jan 9 2019, 8:18 AM
renenkel added a comment to D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .

If you haven't already, need to run Masoud's test program after making your changes.

Jan 9 2019, 8:18 AM

Dec 19 2018

renenkel added a comment to D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .

I don't understand why you need the if statement in SCALE_DOUBLE. Since you are converting to unsigned int128, I don't know why you cast to (unsigned long long) in one case and (long long) in the other. Check this with Masoud, but I think you can do both the same way.
SCALE_DOUBLE deserves more explanatory comments.
doubleType is confusing. It's not a type. It's either 0 or 1 for the high of low double.
ldStructure doesn't appear in the macro arguments, but is assumed, which is confusing.
I've also changed the union fields according to my other comment.
The name SCALE_DOUBLE is misleading, since, although it does scale, it really converts a double to 64-bit int.
Operand order is inconsistent, the output is one of the middle arguments.
How about this?

Dec 19 2018, 10:18 PM
renenkel added a comment to D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .

In the summary, we're not using DD.h any more, so that struct code should be changed to the new version.

Dec 19 2018, 9:24 PM
renenkel requested changes to D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC .

In floattitf_test.c and fixunstfti.c,

Dec 19 2018, 9:12 PM
renenkel requested changes to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

In floattitf_test.c,

Dec 19 2018, 8:45 PM
renenkel added inline comments to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .
Dec 19 2018, 8:39 PM

Dec 6 2018

renenkel added a comment to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .
Dec 6 2018, 2:57 PM
renenkel added a comment to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

Masoud wrote a test program that exercises the conversions over the full range of exponents and compares the results to the gcc conversion. My understanding is that the results matched except for the long double NaN (which is arbitrary), and some long doubles with gaps between the high and low double, for which it was possible to determine that the gcc result was wrong (Masoud, please confirm?)

Dec 6 2018, 2:38 PM
renenkel added a comment to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

Regarding Nemanjai's comment, "I would have expected that every __int128_t value is a number (i.e. not a NAN) and none of them produce an infinity." Correct. No 128-bit int converted to a long double should produce NaN or Inf. However, we have to decide what should happen when a long double NaN or Inf gets converted to a 128-bit int. It seems reasonable that +-Inf gets converted to the largest/smallest int128 of the same sign. But there is not really any natural value for NaN. Unless there is a standard governing this, I would suggest it should be undefined. We've chosen to retain the same bit pattern of the long double NaN in the converted int128. The gcc conversion produces a strange arbitrary number 0x80...080...080...080...0. I'm not sure what the value would be in matching this, but it would be possible with a special case, should that be deemed necessary.

Dec 6 2018, 2:30 PM
renenkel requested changes to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

I think the use of the DD.h include file makes the code confusing, for several reasons: 1. It requires you to look at two files; 2. An additional assignment is required to interpret the component doubles of the long double as 64-bit ints. I suggest you retain the original form of the union, for example:

Dec 6 2018, 2:20 PM

Nov 9 2018

renenkel added a comment to D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC .

The summary doesn't match the code. The code floattitf.c converts from 128-bit int to long double, but the summary says it goes the opposite direction.

Nov 9 2018, 8:47 AM