Merge pull request #2281 from rtfeldman/i/2149

(llvm) Generate code for tag unions less than 64 bits in size correctly

cli/src/repl/eval.rs

@@ -231,17 +231,10 @@ fn jit_to_ast_help<'a>(
                                         main_fn_name,
                                         size as usize,
                                         |ptr: *const u8| {
-                                            // Because this is a `Wrapped`, the first 8 bytes encode the tag ID
+                                            // Because this is a `NonRecursive`, the tag ID is definitely after the data.
-                                            let offset = tags_and_layouts
+                                            let offset = union_layout
-                                                .iter()
+                                                .data_size_without_tag_id(env.ptr_bytes)
-                                                .map(|(_, fields)| {
+                                                .unwrap();
-                                                    fields
-                                                        .iter()
-                                                        .map(|l| l.stack_size(env.ptr_bytes))
-                                                        .sum()
-                                                })
-                                                .max()
-                                                .unwrap_or(0);
 
                                             let tag_id = match union_layout.tag_id_builtin() {
                                                 Builtin::Bool => {

cli/tests/repl_eval.rs

@@ -567,6 +567,23 @@ mod repl_eval {
         );
     }
 
+    #[test]
+    fn issue_2149() {
+        expect_success(r#"Str.toI8 "127""#, "Ok 127 : Result I8 [ InvalidNumStr ]*");
+        expect_success(
+            r#"Str.toI8 "128""#,
+            "Err InvalidNumStr : Result I8 [ InvalidNumStr ]*",
+        );
+        expect_success(
+            r#"Str.toI16 "32767""#,
+            "Ok 32767 : Result I16 [ InvalidNumStr ]*",
+        );
+        expect_success(
+            r#"Str.toI16 "32768""#,
+            "Err InvalidNumStr : Result I16 [ InvalidNumStr ]*",
+        );
+    }
+
     //    #[test]
     //    fn parse_problem() {
     //        // can't find something that won't parse currently

compiler/gen_llvm/src/llvm/convert.rs

@@ -246,14 +246,18 @@ fn block_of_memory_help(context: &Context, union_size: u32) -> BasicTypeEnum<'_>
     let num_i64 = union_size / 8;
     let num_i8 = union_size % 8;
 
+    let i8_array_type = context.i8_type().array_type(num_i8).as_basic_type_enum();
     let i64_array_type = context.i64_type().array_type(num_i64).as_basic_type_enum();
 
-    if num_i8 == 0 {
+    if num_i64 == 0 {
-        // the object fits perfectly in some number of i64's
+        // The object fits perfectly in some number of i8s
+        context.struct_type(&[i8_array_type], false).into()
+    } else if num_i8 == 0 {
+        // The object fits perfectly in some number of i64s
         // (i.e. the size is a multiple of 8 bytes)
         context.struct_type(&[i64_array_type], false).into()
     } else {
-        // there are some trailing bytes at the end
+        // There are some trailing bytes at the end
         let i8_array_type = context.i8_type().array_type(num_i8).as_basic_type_enum();
 
         context

compiler/gen_wasm/src/backend.rs

@@ -830,7 +830,9 @@ impl<'a> WasmBackend<'a> {
         // Store the tag ID (if any)
         if stores_tag_id_as_data {
             let id_offset = data_offset + data_size - data_alignment;
-            let id_align = Align::from(data_alignment);
+
+            let id_align = union_layout.tag_id_builtin().alignment_bytes(PTR_SIZE);
+            let id_align = Align::from(id_align);
 
             self.code_builder.get_local(local_id);
 
@@ -912,7 +914,9 @@ impl<'a> WasmBackend<'a> {
         if union_layout.stores_tag_id_as_data(PTR_SIZE) {
             let (data_size, data_alignment) = union_layout.data_size_and_alignment(PTR_SIZE);
             let id_offset = data_size - data_alignment;
-            let id_align = Align::from(data_alignment);
+
+            let id_align = union_layout.tag_id_builtin().alignment_bytes(PTR_SIZE);
+            let id_align = Align::from(id_align);
 
             self.storage
                 .load_symbols(&mut self.code_builder, &[structure]);

compiler/gen_wasm/src/storage.rs

@@ -519,6 +519,10 @@ impl<'a> Storage<'a> {
                 alignment_bytes,
                 ..
             } => {
+                if self.stack_frame_pointer.is_none() {
+                    self.stack_frame_pointer = Some(self.get_next_local_id());
+                }
+
                 let (to_ptr, to_offset) = location.local_and_offset(self.stack_frame_pointer);
                 copy_memory(
                     code_builder,

compiler/mono/src/layout.rs

@@ -338,14 +338,9 @@ impl<'a> UnionLayout<'a> {
 
     pub fn tag_id_builtin(&self) -> Builtin<'a> {
         match self {
-            UnionLayout::NonRecursive(_tags) => {
+            UnionLayout::NonRecursive(tags) => {
-                // let union_size = tags.len();
+                let union_size = tags.len();
-                // Self::tag_id_builtin_help(union_size)
+                Self::tag_id_builtin_help(union_size)
-
-                // The quicksort-benchmarks version of Quicksort.roc segfaults when
-                // this number is not I64. There must be some dependence on that fact
-                // somewhere in the code, I have not found where that is yet...
-                Builtin::Int(IntWidth::U64)
             }
             UnionLayout::Recursive(tags) => {
                 let union_size = tags.len();
@@ -445,6 +440,27 @@ impl<'a> UnionLayout<'a> {
             None
         };
 
+        self.data_size_and_alignment_help_match(id_data_layout, pointer_size)
+    }
+
+    /// Size of the data before the tag_id, if it exists.
+    /// Returns None if the tag_id is not stored as data in the layout.
+    pub fn data_size_without_tag_id(&self, pointer_size: u32) -> Option<u32> {
+        if !self.stores_tag_id_as_data(pointer_size) {
+            return None;
+        };
+
+        Some(
+            self.data_size_and_alignment_help_match(None, pointer_size)
+                .0,
+        )
+    }
+
+    fn data_size_and_alignment_help_match(
+        &self,
+        id_data_layout: Option<Layout>,
+        pointer_size: u32,
+    ) -> (u32, u32) {
         match self {
             Self::NonRecursive(tags) => {
                 Self::data_size_and_alignment_help(tags, id_data_layout, pointer_size)
@@ -788,7 +804,7 @@ impl<'a, 'b> Env<'a, 'b> {
     }
 }
 
-const fn round_up_to_alignment(width: u32, alignment: u32) -> u32 {
+pub const fn round_up_to_alignment(width: u32, alignment: u32) -> u32 {
     if alignment != 0 && width % alignment > 0 {
         width + alignment - (width % alignment)
     } else {
@@ -934,23 +950,7 @@ impl<'a> Layout<'a> {
                 use UnionLayout::*;
 
                 match variant {
-                    NonRecursive(fields) => {
+                    NonRecursive(_) => variant.data_size_and_alignment(pointer_size).0,
-                        let tag_id_builtin = variant.tag_id_builtin();
-
-                        fields
-                            .iter()
-                            .map(|tag_layout| {
-                                tag_layout
-                                    .iter()
-                                    .map(|field| field.stack_size(pointer_size))
-                                    .sum::<u32>()
-                            })
-                            .max()
-                            .map(|w| round_up_to_alignment(w, tag_id_builtin.alignment_bytes(pointer_size)))
-                            .unwrap_or_default()
-                            // the size of the tag_id
-                            + tag_id_builtin.stack_size(pointer_size)
-                    }
 
                     Recursive(_)
                     | NullableWrapped { .. }
@@ -2795,9 +2795,8 @@ mod test {
 
         let layout = Layout::Union(UnionLayout::NonRecursive(&tt));
 
-        // at the moment, the tag id uses an I64, so
         let ptr_width = 8;
-        assert_eq!(layout.stack_size(ptr_width), 8);
+        assert_eq!(layout.stack_size(ptr_width), 1);
-        assert_eq!(layout.alignment_bytes(ptr_width), 8);
+        assert_eq!(layout.alignment_bytes(ptr_width), 1);
     }
 }

compiler/test_gen/src/gen_tags.rs

@@ -23,10 +23,9 @@ fn width_and_alignment_u8_u8() {
 
     let layout = Layout::Union(UnionLayout::NonRecursive(&tt));
 
-    // at the moment, the tag id uses an I64, so
     let ptr_width = 8;
-    assert_eq!(layout.alignment_bytes(ptr_width), 8);
+    assert_eq!(layout.alignment_bytes(ptr_width), 1);
-    assert_eq!(layout.stack_size(ptr_width), 16);
+    assert_eq!(layout.stack_size(ptr_width), 2);
 }
 
 #[test]
@@ -126,8 +125,7 @@ fn applied_tag_just_enum() {
                 "#
         ),
         (2, 0),
-        (u8, [u8; 7], u8),
+        (u8, u8)
-        |(a, _, c)| (a, c)
     );
 }