C Backend: f16 and f128 support #10771

jean-dao · 2022-02-02T22:04:31Z

128-bit floats: use __float128 type, re-use 128-bit int literal trick for floats
16-bit floats: use either __fp16 or _Float16 when supported

Compiler support is not super robust, but since __int128 is also used without any #if defined guard, I did the same for __float128.

Support for 16-bit floats is a bit more documented from what I found and I tried to make the #if defined guards match actual compiler support. See https://gcc.gnu.org/onlinedocs/gcc/Half-Precision.html and https://clang.llvm.org/docs/LanguageExtensions.html#id12. (I didn't found any doc for the Microsoft compiler.)

Note/Question: The TODO: f128 ? comment in Value.floatToInt() leads me to think there was maybe a reason (performance related?) not to directly use a f128 instead of a f64, so it may require some rework.

matu3ba

Some things need clarification. Better split this in 2 PRs, as this also includes 128bit integer support. Tests for the c codegen them are also missing.

A few more tests would be great, both for floatop.zig and the C side.
floatop.zig has very few test cases, which do not involve negative numbers or the floating point number edge cases.

matu3ba · 2022-02-03T08:41:08Z

src/codegen/c.zig

+        assert(bits <= 128);
+
+        // Clang and GCC don't support 128-bit integer constants but will
+        // hopefully unfold them if we construct one manually.


It would be helpful, if we could test this in CI. Do you have an idea on that?

matu3ba · 2022-02-03T08:44:36Z

src/codegen/c.zig

+        const magnitude = if (val.positive)
+            val.to(u128) catch unreachable
+        else
+            std.math.absCast(val.to(i128) catch unreachable);

        const high = @truncate(u64, magnitude >> 64);
        const low = @truncate(u64, magnitude);

        // (int128_t)/<->( ( (uint128_t)( val_high << 64 )u ) + (uint128_t)val_low/u )


This reads like it is little endian only.
Make a comptime error for big-endian or clarify.

Note that I didn't write the int128 handling code, just modifying the function to also accept smaller values. I can put a TODO comment, but I think it should be part of another PR.

Better would be a compilation error with comptime check of the endianess.

matu3ba · 2022-02-03T09:04:21Z

src/codegen/c.zig

@@ -386,52 +386,97 @@ pub const DeclGen = struct {
        try dg.renderDeclName(decl, writer);
    }

-    fn renderInt128(
+    fn renderBigIntConst(


What is the reason to rename it like it would accept any integer sizes and then do assert(bits <= 128); in the fn body?

The reason I did those changes (renderInt128/renderBigIntConst refactoring/renaming) is not obvious anymore, I'll remove unneeded changes.

matu3ba · 2022-02-03T09:06:31Z

src/codegen/c.zig


        return writer.writeByte(')');
    }

-    fn renderBigIntConst(
+    fn renderInt128(


I dont understand why this fn is necessary.

The reason I did those changes (renderInt128/renderBigIntConst refactoring/renaming) is not obvious anymore, I'll remove unneeded changes.

jean-dao · 2022-02-03T12:51:58Z

Some things need clarification. Better split this in 2 PRs, as this also includes 128bit integer support. Tests for the c codegen them are also missing.

I didn't write the 128-bit integer support, this was already here. I realize there are a bunch of unneeded changes in this PR, so I'll clean it up. I did have to change a bit the 128-bit int function to handle smaller values as well.

A few more tests would be great, both for floatop.zig and the C side. floatop.zig has very few test cases, which do not involve negative numbers or the floating point number edge cases.

Agreed, will write more tests.

Thanks for the review!

jean-dao · 2022-02-04T17:10:28Z

I updated my branch with the following changes:

128-bit floats are not available on all platform (e.g. not available on freebsd or macos), don't use __float128 type in this case
remove some unnecessary changes (functions renaming and such)
add more tests
fix an unrelated bug I found when writing tests (829ea92)
add a comptime error when using renderInt128() on big endian arch. I'm not 100% sure this is actually needed, I will look more into it.

drew-gpf · 2022-02-04T18:38:55Z

Hi, I wrote the original renderInt128(). The endianness check should not be needed because you're just taking integer values and shifting them; the underlying representation only matters when you put it into memory, although given that there's doubt about it it would be worth it if someone could test this with a big endian arch.

matu3ba · 2022-03-14T23:05:37Z

@jean-dao Can you rebase to resolve the conflicts + rerun CI, which hopefully passes this time?

jmc-88 · 2022-03-16T09:42:11Z

test/behavior/floatop.zig

@@ -697,3 +697,38 @@ test "f128 at compile time is lossy" {

    try expect(@as(f128, 10384593717069655257060992658440192.0) + 1 == 10384593717069655257060992658440192.0);
 }
+
+test "f16 literals" {
+    if (builtin.zig_backend != .stage2_c) return error.SkipZigTest; // TODO


Why are this test and the next being skipped for the C backend? I would assume them to be passing after your changes.

You're right, only the f128 tests are not passing, I'll update again.

stage2/bin/zig test test/behavior.zig -I test -target aarch64-linux --test-cmd qemu-aarch64 --test-cmd-bin test/behavior/floatop.zig:701:1: error: TODO implement const of type 'f16'

Actually it's not passing, although for a different reason than f128. So I think it should still be skipped for backend != c. It's hard to tell for me which backend is actually supported apart from C.

topolarity · 2022-03-16T19:49:09Z

src/codegen/c.zig

+        if (std.math.isNan(v) or std.math.isInf(v)) {
+            // just generate a bit cast (exactly like we do in airBitcast)
+            switch (T) {
+                f16 => return writer.print("zig_bitcast_f16_u16(0x{x})", .{v}),


Is a bitCast of v missing here?

I think @topolarity is right, without the @bitCast() this will print the float to hex as is, resulting in something like 0x0x1.ecp6: https://zig.godbolt.org/z/9ehccKWW3

matu3ba

add a few examples to test the relevant cases.

matu3ba · 2022-03-18T12:34:51Z

src/link/C/zig.h

+#define ZIG_HAS_FLOAT128
+#define float128_t long double
+#elif defined(__FreeBSD__) || defined(__APPLE__)
+#define float128_t float128_t_is_not_supported_on_this_target


Just curious: Why not #error float128_t_is_not_supported_on_this_target ?

+1 to @matu3ba's point, please use:

#error "float128_t is not supported on this target"

From my understanding, using #error will make all compilations fail on unsupported targets, even if float128_t is not used. I agree this is less clear though.

Can you attach a TODO and make this something along ZIG_UNSUPPORTED_SYMBOL_ON_TARGET_float128_t ?
It would be nice to enable a central example header for the user to invoke such that the user can check these kind of things and also includes stuff like abi checks as macros: marler8997/ziglibc#1

Getting a convention for what stuff is unsupported + make it easily checkable and the checks editable sounds very relevant to me.

matu3ba · 2022-03-18T12:35:08Z

src/link/C/zig.h

+#define ZIG_HAS_FLOAT16
+#define float16_t _Float16
+#else
+#define float16_t float16_t_is_not_supported_on_this_target


same question: Why not #error float128_t_is_not_supported_on_this_target ?

matu3ba · 2022-03-18T12:46:18Z

test/behavior/floatop.zig

+
+    var buf: f16 = math.f16_min;
+    try expect(math.f16_min == buf);
+


missing test cases +-inf, -0 with at least 1 example with negative sign.
Not sure, if you also want to check f16_true_min or what the expected behavior for denormals is.

matu3ba · 2022-03-18T12:47:28Z

test/behavior/floatop.zig

+    if (builtin.zig_backend != .stage2_c) return error.SkipZigTest; // TODO
+
+    const f128_min = @bitCast(f128, @as(u128, 0x00010000000000000000000000000000));
+    const f128_max = @bitCast(f128, @as(u128, 0x7FFEFFFFFFFFFFFFFFFFFFFFFFFFFFFF));


same story: at least 1 example with negative sign.

andrewrk

Thanks for your patch. @topolarity's review comment looks like a merge blocker to me and I found a few other issues as well-

andrewrk · 2022-03-19T19:38:01Z

test/behavior/floatop.zig

+    try expect(@as(f16, 0) == buf);
+}
+
+test "f128 tests" {


Can you please be more specific? What exactly is this testing?

src/codegen/c.zig

test/behavior/floatop.zig

jmc-88 · 2022-03-19T19:46:33Z

src/codegen/c.zig

+        if (std.math.isNan(v) or std.math.isInf(v)) {
+            // just generate a bit cast (exactly like we do in airBitcast)
+            switch (T) {
+                f16 => return writer.print("zig_bitcast_f16_u16(0x{x})", .{v}),


I think @topolarity is right, without the @bitCast() this will print the float to hex as is, resulting in something like 0x0x1.ecp6: https://zig.godbolt.org/z/9ehccKWW3

src/codegen/c.zig

jmc-88 · 2022-03-19T20:00:19Z

src/link/C/zig.h

+#define ZIG_HAS_FLOAT128
+#define float128_t long double
+#elif defined(__FreeBSD__) || defined(__APPLE__)
+#define float128_t float128_t_is_not_supported_on_this_target


+1 to @matu3ba's point, please use:

#error "float128_t is not supported on this target"

jean-dao · 2022-04-01T14:10:57Z

The new update changelog:

use bitcast to unsigned int type before printing hex value in renderSmallFloat
more test values (inf, -inf, 0, -0, -1, nan, min, true_min, max, -max)
better test names
handle C limited support for f16: C (well at least clang) only supports f16 as a storage type, so it cannot be used for e.g. function return types or parameters. I choose to use f32 (float) instead in such cases.

jmc-88 · 2022-04-02T23:48:27Z

The CI run crashed. ☹️ Can you reproduce the crash locally?

jean-dao · 2022-04-03T13:41:07Z

@jmc-88 I cannot reproduce it locally. I thought maybe it could be related to musl libc vs glibc, but I couldn't yet reproduce the ci setup on my local setup. Since there is no output from the zig run subcommand that fail it's hard to know how to debug :/

topolarity · 2022-04-03T15:42:10Z

I was able to reproduce the failure locally. This is the failing test case from basic.zig:

const builtin = @import("builtin");
const expect = @import("std").testing.expect;

test "take address of parameter" {
    try testTakeAddressOfParameter(12.34);
}
fn testTakeAddressOfParameter(f: f32) !void {
    const f_ptr = &f;
    try expect(f_ptr.* == 12.34);
}

Looks like the problem is that 12.34 is rendered in decimal fp 1.23400001e+01 instead of hex fp 0x1.8ae148p3, causing a rounding error.

jean-dao · 2022-04-03T20:18:37Z

@topolarity thank you for your help!

On another topic, it seems the f128 tests are segfaulting on i386. I'm not quite sure how it passed before, I couldn't reproduce it (= passing test), even on an older master branch. I made the f128 test skipped on i386, not sure if that is acceptable.

topolarity · 2022-04-04T20:20:20Z

Good news, it doesn't like the segfault is related to these changes! 👍

Bug filed here, with root cause: #11383

andrewrk · 2022-04-22T15:44:36Z

src/codegen/c.zig

        comptime assert(int_info.bits > 64 and int_info.bits <= 128);

+        // TODO support big endian arch
+        comptime assert(builtin.cpu.arch.endian() == .Little);


This is checking the endianness of the compiler, not the target. Also can you just fix the bug, if any, rather than asserting? It's not that hard to solve.

andrewrk · 2022-04-22T15:47:05Z

src/link/C/zig.h

+#define ZIG_HAS_FLOAT128
+#define float128_t long double
+#elif defined(__FreeBSD__) || defined(__APPLE__)
+#define float128_t ZIG_UNSUPPORTED_SYMBOL_ON_TARGET_float128_t


In this case make it a 128 bit integer and emit instructions to perform softfloat on it. All the softfloat functions are available in compiler-rt.

andrewrk · 2022-05-11T19:54:08Z

This PR seems to be abandoned. Please open a fresh one if you want to continue the effort.

matu3ba suggested changes Feb 3, 2022

View reviewed changes

jean-dao force-pushed the c_backend_floats branch from 50665a9 to 10c47c5 Compare February 4, 2022 17:02

jean-dao force-pushed the c_backend_floats branch 2 times, most recently from 096ca20 to c796979 Compare March 15, 2022 22:03

jmc-88 reviewed Mar 16, 2022

View reviewed changes

jean-dao force-pushed the c_backend_floats branch 2 times, most recently from 8492f51 to e03aabf Compare March 16, 2022 10:22

topolarity reviewed Mar 16, 2022

View reviewed changes

matu3ba suggested changes Mar 18, 2022

View reviewed changes

jmc-88 mentioned this pull request Mar 19, 2022

CBE: implement mod, divFloor, divTrunc, and enable more passing tests #11229

Merged

andrewrk requested changes Mar 19, 2022

View reviewed changes

jmc-88 reviewed Mar 19, 2022

View reviewed changes

jean-dao added 2 commits April 1, 2022 09:47

C backend: fix zig_bitcast_f64_u64() return type

70f98d5

C backend: 128-bit ints on big endian arch is not yet supported

385307e

jean-dao force-pushed the c_backend_floats branch 3 times, most recently from 98e008b to 34ecec5 Compare April 1, 2022 14:06

jean-dao force-pushed the c_backend_floats branch from 34ecec5 to 5358aed Compare April 1, 2022 14:20

jean-dao force-pushed the c_backend_floats branch 3 times, most recently from 2b9091c to 1d7af0c Compare April 3, 2022 16:38

C backend: better f128 support

de241bb

C backend: better f16 support

243e160

jean-dao force-pushed the c_backend_floats branch from 1d7af0c to 243e160 Compare April 3, 2022 17:07

jean-dao requested a review from andrewrk April 4, 2022 10:13

andrewrk requested changes Apr 22, 2022

View reviewed changes

andrewrk closed this May 11, 2022


		var buf: f16 = math.f16_min;
		try expect(math.f16_min == buf);

C Backend: f16 and f128 support #10771

C Backend: f16 and f128 support #10771

Conversation

jean-dao commented Feb 2, 2022

matu3ba left a comment • edited Loading

Choose a reason for hiding this comment

matu3ba Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matu3ba Feb 3, 2022 • edited Loading

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

jean-dao commented Feb 3, 2022

jean-dao commented Feb 4, 2022

drew-gpf commented Feb 4, 2022

matu3ba commented Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-dao Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matu3ba left a 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

matu3ba Mar 25, 2022 • edited Loading

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

andrewrk left a 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

jean-dao commented Apr 1, 2022 • edited Loading

jmc-88 commented Apr 2, 2022

jean-dao commented Apr 3, 2022

topolarity commented Apr 3, 2022

jean-dao commented Apr 3, 2022

topolarity commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk commented May 11, 2022

matu3ba left a comment •

edited

Loading

matu3ba Feb 3, 2022 •

edited

Loading

matu3ba Feb 3, 2022 •

edited

Loading

matu3ba commented Mar 14, 2022 •

edited

Loading

jean-dao Mar 16, 2022 •

edited

Loading

matu3ba Mar 25, 2022 •

edited

Loading

jean-dao commented Apr 1, 2022 •

edited

Loading