Skip to content

Add Float16Array #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 3, 2024
Merged

Add Float16Array #491

merged 4 commits into from
Sep 3, 2024

Conversation

bnoordhuis
Copy link
Contributor

No description provided.

d += 1;
e -= 15;
}
d = scalbn(d, e);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a way to implement fromfp16() and tofp16() without using functions from math.h, I'd love to hear it!

My initial approach used plain bit shifting but I couldn't get rounding to work correctly in all cases. Ex. test262 expects Math.fp16round(32767) to round up to 32,768.

For anyone interested, here's my first attempt:

static inline double fromfp16(uint16_t v) {
    union {
        uint16_t v;
        struct {
            uint16_t f:10;
            uint16_t e:5;
            uint16_t s:1;
        } x;
    } h;
    union {
        double v;
        struct {
            uint64_t f:52;
            uint64_t e:11;
            uint64_t s:1;
        } x;
    } q;
    h.v = v;
    if (h.x.e == 31) { // +/-Infinity, NaN
        q.x.f = h.x.f > 0;
        q.x.e = 2047;
    } else if (h.x.e == 0 && h.x.f == 0) { // TODO subnormal if h.x.f > 0
        q.x.f = 0;
        q.x.e = 0;
    } else {
        q.x.f = (uint64_t)h.x.f << 42;
        q.x.e = (h.x.e - 15) + 1023;
    }
    q.x.s = h.x.s;
    return q.v;
}

static inline uint16_t tofp16(double d) {
    union {
        uint16_t v;
        struct {
            uint16_t f:10;
            uint16_t e:5;
            uint16_t s:1;
        } x;
    } h;
    union {
        double v;
        struct {
            uint64_t f:52;
            uint64_t e:11;
            uint64_t s:1;
        } x;
    } q;
    q.v = d;
    if (q.x.e == 2047)
        return 0x7C00 | (q.x.s << 15) | (q.x.f > 0); // +/-Infinity, NaN
    if (d < -65504 || d > 65504)
        return 0x7C00 | (q.x.s << 15); // out of range, return Infinity
    if (q.x.e == 0 && q.x.f == 0) {
        return 0;
    }
    h.x.f = q.x.f >> 42;
    h.x.e = (q.x.e - 1023) + 15;
    h.x.s = q.x.s;
    return h.v;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really want to avoid the dependency on math.h for this very function perhaps we can borrow it from musl? https://git.musl-libc.org/cgit/musl/tree/src/math/scalbn.c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I see we use more math functions below.

test262/test/built-ins/TypedArrayConstructors/internals/Set/key-is-canonical-invalid-index-prototype-chain-set.js:35: Test262Error: value should not be coerced Expected SameValue(«110», «0») to be true
test262/test/built-ins/TypedArrayConstructors/internals/Set/key-is-canonical-invalid-index-prototype-chain-set.js:35: strict mode: Test262Error: value should not be coerced Expected SameValue(«110», «0») to be true
test262/test/built-ins/TypedArrayConstructors/internals/Set/key-is-canonical-invalid-index-reflect-set.js:35: Test262Error: value should not be coerced Expected SameValue(«160», «0») to be true
test262/test/built-ins/TypedArrayConstructors/internals/Set/key-is-canonical-invalid-index-reflect-set.js:35: strict mode: Test262Error: value should not be coerced Expected SameValue(«160», «0») to be true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests were already failing, it's just that the failure count is different now because Float16Array is also tested.

@@ -356,6 +357,81 @@ static inline void inplace_bswap32(uint8_t *tab) {
put_u32(tab, bswap32(get_u32(tab)));
}

static inline double fromfp16(uint16_t v) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does one find the rules you followed here? (trying to inform myself and do a good review 😅 )

Copy link
Contributor Author

@bnoordhuis bnoordhuis Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly derived it from first principles; they're identical to floats and doubles, except with smaller exponents and mantissas (5 and 10 bits resp. with an exponent bias of 15. Compare doubles: 11 and 52 bits, bias of 1023.)

To get the floating point value, you compute pow(2, exponent-15) * (1 + mantissa/1024.0). Compare doubles: pow(2, exponent-1023) * (1 + mantissa/pow(2, 52))

To go from double to half-float or vice versa... my intuition was that exponent - 1023 + 15 and mantissa >> (52 - 10) would be Good Enough(TM) but alas, it wasn't, so now I'm mostly computing it in the double-precision domain.

The Wikipedia page is okay. (edit: what I call 'mantissa', wikipedia calls 'fraction')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and if you're worried about correctness: test262 has pretty good coverage so it should be okay. Everything passes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bnoordhuis
Copy link
Contributor Author

I discovered today that V8 uses https://github.com/Maratyszcza/FP16/blob/master/include/fp16/fp16.h

Maybe that's an okay alternative: MIT-licensed, header-only, uses intrinsics and bit shifts (see! possible after all)

On the other hand: external dependency, more code, operates on floats instead of doubles so maybe not all that more efficient after all (because we'd be casting those floats to/from doubles everywhere)

@saghul
Copy link
Contributor

saghul commented Sep 3, 2024

No strong opinion, but I feel like your current approach better fits the project here.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

d += 1;
e -= 15;
}
d = scalbn(d, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really want to avoid the dependency on math.h for this very function perhaps we can borrow it from musl? https://git.musl-libc.org/cgit/musl/tree/src/math/scalbn.c

static inline double fromfp16(uint16_t v) {
double d, s;
int e;
if ((v & 0x7C00) == 0x7C00) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal preference probably, but I'd define this like FP6_INF or something, for readability...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but... for doubles there's INFINITY from math.h and you can write -INFINITY to get a negative infinity. For FP16_INFINITY that doesn't work because -0x7C00 is 0x8400 and that's a regular number. I don't like that lack of parity.

d += 1;
e -= 15;
}
d = scalbn(d, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I see we use more math functions below.

@bnoordhuis
Copy link
Contributor Author

If we really want to avoid the dependency on math.h for this very function perhaps we can borrow it from musl?

I initially had something like that. I was working out the logic by hand and after a while I realized I'd written an open-coded scalbn!

(Poorly though, without much thought given to subnormals.)

@bnoordhuis bnoordhuis merged commit 9bc41a8 into quickjs-ng:master Sep 3, 2024
50 checks passed
@bnoordhuis bnoordhuis deleted the float16array branch September 3, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants