compiler_rt f128 support additions #1166
Conversation
| const aExp: u32 = aAbs >> srcSigBits; | ||
| const shift: u32 = srcExpBias - dstExpBias - aExp + 1; | ||
| const aExp = aAbs >> srcSigBits; | ||
| const shift = srcExpBias - dstExpBias - aExp + 1; |
There was a problem hiding this comment.
Suggestion: @truncate(u32, ...)? The exponent and the shift fit in 32 bits.
I put in the u32 intentionally because I figured that for performance reasons you don't want to use a 64 or 128 bits type when you can avoid it.
There was a problem hiding this comment.
I can add an @intCast(u32, ...) here. I omitted it here for the moment since the narrowing doesn't work implicitly for the longer src_t types (such as f128).
| test__truncsfhf2(0x33000000, 0x0000); // 0x1.0p-25 -> zero | ||
| test__truncsfhf2(0x35800000, 0x0010); // denormal, 0x1.0p-20 | ||
| test__truncsfhf2(0x33280000, 0x0001); // denormal, 0x1.5p-25 -> 0x1.0p-24 | ||
| test__truncsfhf2(0x33000000, 0x0000); // 0x1.0p-25 -> zero |
There was a problem hiding this comment.
Is this a zig fmt thing? Lots of whitespace churn otherwise.
There was a problem hiding this comment.
Yes, that is from zig fmt.
| @@ -0,0 +1,28 @@ | |||
| const builtin = @import("builtin"); | |||
| const is_test = builtin.is_test; | |||
| const std = @import("../../index.zig"); | |||
There was a problem hiding this comment.
Will remove this import and hardcode the mantissa bits where needed to avoid dragging in all of std on test.
|
|
||
| const mantissa_bits = std.math.floatMantissaBits(f128); | ||
| const exponent_bits = std.math.floatExponentBits(f128); | ||
| const exponent_bias = (1 << (exponent_bits - 1)) - 1; |
There was a problem hiding this comment.
Is this missing a right shift? This looks like the max exponent, not the bias.
floatunsitf.zig, line 14: same question.
There was a problem hiding this comment.
This is correct since we halve the range. For example, a double with exponent bit count of 11 gives (1 << (11 - 1)) - 1 = 1023. Max exponent is (1 << 11) - 1.
There was a problem hiding this comment.
Sorry, you're right. I don't remember what I was thinking at the time but the logic looks Obviously Correct to me now.
|
|
||
| const mantissa_bits = std.math.floatMantissaBits(f128); | ||
| const exponent_bits = std.math.floatExponentBits(f128); | ||
| const exponent_bias = (1 << (exponent_bits - 1)) - 1; |
There was a problem hiding this comment.
Sorry, you're right. I don't remember what I was thinking at the time but the logic looks Obviously Correct to me now.
| result += (@intCast(u128, exp) + exponent_bias) << mantissa_bits; | ||
|
|
||
| return @bitCast(f128, result); | ||
| } |
There was a problem hiding this comment.
Perhaps this can be shared with floatunditf.zig?
| }, | ||
| FLT_MANT_DIG + 2 => {}, | ||
| else => { | ||
| const shift_amt = @bitCast(i32, N +% (FLT_MANT_DIG + 2)) -% sd; |
There was a problem hiding this comment.
For clarity: N + FLT_MANT_DIG + 2)? When I see %+, I immediately go scan for possible overflow inputs but there can't be any in this case, can it? I'd use wraparound operators sparingly.
Line 14: same comment. Also maybe line 32, 38, 43, etc.
There was a problem hiding this comment.
I agree. For the moment I used existing compiler_rt functions as a template and have done minimal changes to the actual structure. I'll do a more in-depth scan when complete and make these sorts of things more idiomatic/correct.
|
This is now complete, will merge when tests have run. Few notes:
|
|
Great, thanks. I noticed that "fix" of mine didn't work so I'll revert it here and we can use the one from your PR instead. |
See #495.
Pending completion, but since nothing is blocking this anymore should be done in no time.