Support more bit array options for ints and floats on JavaScript #3293

richard-viney · 2024-06-19T04:12:44Z

This PR adds support for the following options for floats and ints in bit array patterns & expressions on the JavaScript target:

Explicit size on floats: 64-bit and 32-bit. 16-bit floats aren't included, though they are supported by Erlang.
Explicit endianness on floats and ints via big and little options. native endianness hasn't been added, but potentially could be. big remains the default.
Explicit signedness on ints via signed and unsigned.

Implementation notes:

There was some pre-existing code duplication in expression.rs for bit arrays in constant expressions. This PR hasn't aimed to fix this, and there's now a bit more code duplication of a similar nature in expression.rs as a result.
There was one place where it was incorrectly assumed that the JavaScript platform is little endian (in the use of Float64Array). Most clients these days are indeed little endian, so it wasn't much of a problem in practice, but I've changed the code to use DataView instead.
There are non-backwards compatible changes to function signatures in the JavaScript prelude, but affected functions were all tagged with // @internal so I assumed that it's ok to change these.

lpil

Awesome! Thank you! I've left some notes inline.

lpil · 2024-06-20T11:36:41Z

compiler-core/src/javascript/expression.rs

+                        let default_size = if segment.type_ == crate::type_::int() {
+                            8usize
+                        } else {
+                            64usize


Shouldn't this only be for floats?

I think ints have an 8-bit size by default?

compiler-core/src/javascript/expression.rs

lpil · 2024-06-20T11:40:49Z

compiler-core/src/javascript/pattern.rs

+    IntFromSlice(usize, usize, bool, bool),
+    FloatFromSlice(usize, usize, bool),


Convert these to struct variants IntFromSlice { a: ..., b: ...} please as I don't understand what the 4 unnamed fields are here

lpil · 2024-06-20T11:41:23Z

compiler-core/src/javascript/pattern.rs

    }

-    fn push_float_at(&mut self, i: usize) {
-        self.path.push(Index::FloatAt(i));
+    fn push_float_from_slice(&mut self, i: usize, end: usize, is_big_endian: bool) {


Let's use an enum for endianness

Done! I wasn't totally sure where to put it, so it's in its own file. Please let me know if it belongs better somewhere else.

lpil · 2024-06-20T11:42:12Z

compiler-core/src/javascript/pattern.rs

-                            self.pop();
-                            offset.increment(1);
-                            Ok(())
+                    if segment.type_ == crate::type_::int()


Same here as above with doing it in two passes

Done as per previous comment

lpil · 2024-06-20T11:43:27Z

compiler-core/src/javascript/tests/bit_arrays.rs

@@ -50,6 +50,24 @@ fn float() {
        r#"
 fn go() {
  <<1.1:float>>
+  <<1.1:float-big>>
+  <<1.1:float-little>>


Each test should only test one thing, so make a new test for each of these new examples please 🙏

lpil · 2024-06-20T11:45:06Z

compiler-core/templates/prelude.mjs

-    const msg = `Bit arrays must be byte aligned on JavaScript, got size of ${size} bits`;
-    throw new globalThis.Error(msg);
-  }
+


Put back the byte alignment check please 🙏

Done. I thought it was no longer needed since the check added in 6d168ef, but reading the commit more carefully I was 100% wrong about that.

lpil · 2024-06-20T11:47:15Z

compiler-core/templates/prelude.mjs

+    return view.getFloat64(start, !isBigEndian)
+  } else if (byteSize === 4) {
+    return view.getFloat32(start, !isBigEndian)
+  }


This function returns undefined when byteSize is not 8 or 4. It must always return a valid value.

Rather than have an end we could have a second bool for whether it is 32 or 64 bits.

I'm considering adding 16-bit floats to JS as well in a separate PR, hence the usage of start & end here to match byteArrayToInt and also allow for 16-bit floats.

Bit array pattern matching on JS doesn't yet allow dynamic sizing, i.e. the size must be a compile-time const, and existing checks ensure that it is 16, 32, or 64.

I've added a check to the JS side so there's a compile error if a float size of 16 is given, as this isn't currently supported.

Given those two points, byteArrayToFloat can't ever be called with a byte size other than 4 or 8. For safety, I've added a check that throws an error to avoid undefined being silently returned, but it shouldn't be possible to ever get there.

Thoughts?

lpil · 2024-06-20T11:47:59Z

compiler-core/templates/prelude.mjs

-export function float64Bits(float) {
-  return new Uint8Array(Float64Array.from([float]).buffer).reverse();
+export function sizedFloat(float, size, isBigEndian) {
+  if (size !== 32 && size !== 64) {


Let's use a bool as that forces it to be one or the other.

I don't think that's an option because the size can be specified dynamically:

let a = int.random(1000) let b = <<1.0:float-size(a)>>

Both Erlang and JS throw an error if the size at runtime isn't supported for floats.

richard-viney

Thanks for the review, all comments have been addressed or responded to, ready for another look 👍

compiler-core/src/javascript/expression.rs

richard-viney · 2024-06-21T01:21:46Z

compiler-core/src/javascript/expression.rs

+                        let default_size = if segment.type_ == crate::type_::int() {
+                            8usize
+                        } else {
+                            64usize


I think ints have an 8-bit size by default?

richard-viney · 2024-06-21T01:22:01Z

compiler-core/src/javascript/pattern.rs

+    IntFromSlice(usize, usize, bool, bool),
+    FloatFromSlice(usize, usize, bool),


richard-viney · 2024-06-21T01:22:32Z

compiler-core/src/javascript/pattern.rs

    }

-    fn push_float_at(&mut self, i: usize) {
-        self.path.push(Index::FloatAt(i));
+    fn push_float_from_slice(&mut self, i: usize, end: usize, is_big_endian: bool) {


Done! I wasn't totally sure where to put it, so it's in its own file. Please let me know if it belongs better somewhere else.

richard-viney · 2024-06-21T01:28:00Z

compiler-core/templates/prelude.mjs

-    const msg = `Bit arrays must be byte aligned on JavaScript, got size of ${size} bits`;
-    throw new globalThis.Error(msg);
-  }
+


Done. I thought it was no longer needed since the check added in 6d168ef, but reading the commit more carefully I was 100% wrong about that.

richard-viney · 2024-06-21T01:59:58Z

compiler-core/templates/prelude.mjs

+    return view.getFloat64(start, !isBigEndian)
+  } else if (byteSize === 4) {
+    return view.getFloat32(start, !isBigEndian)
+  }


I'm considering adding 16-bit floats to JS as well in a separate PR, hence the usage of start & end here to match byteArrayToInt and also allow for 16-bit floats.

Bit array pattern matching on JS doesn't yet allow dynamic sizing, i.e. the size must be a compile-time const, and existing checks ensure that it is 16, 32, or 64.

I've added a check to the JS side so there's a compile error if a float size of 16 is given, as this isn't currently supported.

Given those two points, byteArrayToFloat can't ever be called with a byte size other than 4 or 8. For safety, I've added a check that throws an error to avoid undefined being silently returned, but it shouldn't be possible to ever get there.

Thoughts?

richard-viney · 2024-06-21T02:03:12Z

compiler-core/templates/prelude.mjs

-export function float64Bits(float) {
-  return new Uint8Array(Float64Array.from([float]).buffer).reverse();
+export function sizedFloat(float, size, isBigEndian) {
+  if (size !== 32 && size !== 64) {


I don't think that's an option because the size can be specified dynamically:

let a = int.random(1000) let b = <<1.0:float-size(a)>>

Both Erlang and JS throw an error if the size at runtime isn't supported for floats.

richard-viney · 2024-06-21T02:06:35Z

compiler-core/src/javascript/pattern.rs

-                            self.pop();
-                            offset.increment(1);
-                            Ok(())
+                    if segment.type_ == crate::type_::int()


Done as per previous comment

richard-viney · 2024-06-21T02:06:50Z

compiler-core/src/javascript/tests/bit_arrays.rs

@@ -50,6 +50,24 @@ fn float() {
        r#"
 fn go() {
  <<1.1:float>>
+  <<1.1:float-big>>
+  <<1.1:float-little>>


richard-viney changed the title ~~Int/float size, endianness, and signed~~ Support more bit array options for ints and floats on JavaScript Jun 19, 2024

richard-viney force-pushed the js_float_int_bit_array_options branch from b71d160 to 773c3d0 Compare June 19, 2024 04:16

lpil reviewed Jun 20, 2024

View reviewed changes

richard-viney force-pushed the js_float_int_bit_array_options branch from 773c3d0 to 513f386 Compare June 21, 2024 01:10

richard-viney commented Jun 21, 2024

View reviewed changes

richard-viney force-pushed the js_float_int_bit_array_options branch 3 times, most recently from 544ad7b to 751dee0 Compare June 28, 2024 22:56

Support more bit array options for ints and floats on JavaScript

55d0cd5

richard-viney force-pushed the js_float_int_bit_array_options branch from 751dee0 to 55d0cd5 Compare July 1, 2024 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more bit array options for ints and floats on JavaScript #3293

Support more bit array options for ints and floats on JavaScript #3293

richard-viney commented Jun 19, 2024

lpil left a comment

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

lpil Jun 20, 2024

richard-viney Jun 21, 2024

richard-viney left a comment

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

richard-viney Jun 21, 2024

		IntFromSlice(usize, usize, bool, bool),
		FloatFromSlice(usize, usize, bool),

Support more bit array options for ints and floats on JavaScript #3293

Are you sure you want to change the base?

Support more bit array options for ints and floats on JavaScript #3293

Conversation

richard-viney commented Jun 19, 2024

lpil 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-viney 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

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