Merge pull request #1686 from rtfeldman/refcounting-cleanup

Refcounting cleanup

compiler/gen_llvm/src/llvm/build.rs

@@ -3,15 +3,15 @@ use std::path::Path;
 
 use crate::llvm::bitcode::{call_bitcode_fn, call_void_bitcode_fn};
 use crate::llvm::build_dict::{
-    dict_contains, dict_difference, dict_empty, dict_get, dict_insert, dict_intersection,
+    self, dict_contains, dict_difference, dict_empty, dict_get, dict_insert, dict_intersection,
     dict_keys, dict_len, dict_remove, dict_union, dict_values, dict_walk, set_from_list,
 };
 use crate::llvm::build_hash::generic_hash;
 use crate::llvm::build_list::{
-    allocate_list, empty_list, empty_polymorphic_list, list_append, list_concat, list_contains,
-    list_drop, list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len,
-    list_map, list_map2, list_map3, list_map_with_index, list_prepend, list_range, list_repeat,
-    list_reverse, list_set, list_single, list_sort_with, list_swap,
+    self, allocate_list, empty_list, empty_polymorphic_list, list_append, list_concat,
+    list_contains, list_drop, list_get_unsafe, list_join, list_keep_errs, list_keep_if,
+    list_keep_oks, list_len, list_map, list_map2, list_map3, list_map_with_index, list_prepend,
+    list_range, list_repeat, list_reverse, list_set, list_single, list_sort_with, list_swap,
 };
 use crate::llvm::build_str::{
     empty_str, str_concat, str_count_graphemes, str_ends_with, str_from_float, str_from_int,
@@ -2305,32 +2305,6 @@ fn list_literal<'a, 'ctx, 'env>(
     }
 }
 
-fn decrement_with_size_check<'a, 'ctx, 'env>(
-    env: &Env<'a, 'ctx, 'env>,
-    parent: FunctionValue<'ctx>,
-    size: IntValue<'ctx>,
-    layout: Layout<'a>,
-    refcount_ptr: PointerToRefcount<'ctx>,
-) {
-    let not_empty = env.context.append_basic_block(parent, "not_null");
-
-    let done = env.context.append_basic_block(parent, "done");
-
-    let is_empty =
-        env.builder
-            .build_int_compare(IntPredicate::EQ, size, size.get_type().const_zero(), "");
-
-    env.builder
-        .build_conditional_branch(is_empty, done, not_empty);
-
-    env.builder.position_at_end(not_empty);
-
-    refcount_ptr.decrement(env, &layout);
-
-    env.builder.build_unconditional_branch(done);
-    env.builder.position_at_end(done);
-}
-
 pub fn build_exp_stmt<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
     layout_ids: &mut LayoutIds<'a>,
@@ -2540,34 +2514,25 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>(
                     let (value, layout) = load_symbol_and_layout(scope, symbol);
 
                     match layout {
-                        Layout::Builtin(Builtin::List(_)) => {
+                        Layout::Builtin(Builtin::List(element_layout)) => {
                             debug_assert!(value.is_struct_value());
+                            let alignment = element_layout.alignment_bytes(env.ptr_bytes);
 
-                            // because of how we insert DECREF for lists, we can't guarantee that
-                            // the list is non-empty. When the list is empty, the pointer to the
-                            // elements is NULL, and trying to get to the RC address will
-                            // underflow, causing a segfault. Therefore, in this case we must
-                            // manually check that the list is non-empty
-                            let refcount_ptr = PointerToRefcount::from_list_wrapper(
-                                env,
-                                value.into_struct_value(),
-                            );
-
-                            let length = list_len(env.builder, value.into_struct_value());
-
-                            decrement_with_size_check(env, parent, length, *layout, refcount_ptr);
+                            build_list::decref(env, value.into_struct_value(), alignment);
                         }
-                        Layout::Builtin(Builtin::Dict(_, _)) | Layout::Builtin(Builtin::Set(_)) => {
+                        Layout::Builtin(Builtin::Dict(key_layout, value_layout)) => {
                             debug_assert!(value.is_struct_value());
+                            let alignment = key_layout
+                                .alignment_bytes(env.ptr_bytes)
+                                .max(value_layout.alignment_bytes(env.ptr_bytes));
 
-                            let refcount_ptr = PointerToRefcount::from_list_wrapper(
-                                env,
-                                value.into_struct_value(),
-                            );
+                            build_dict::decref(env, value.into_struct_value(), alignment);
+                        }
+                        Layout::Builtin(Builtin::Set(key_layout)) => {
+                            debug_assert!(value.is_struct_value());
+                            let alignment = key_layout.alignment_bytes(env.ptr_bytes);
 
-                            let length = dict_len(env, scope, *symbol).into_int_value();
-
-                            decrement_with_size_check(env, parent, length, *layout, refcount_ptr);
+                            build_dict::decref(env, value.into_struct_value(), alignment);
                         }
 
                         _ if layout.is_refcounted() => {

compiler/gen_llvm/src/llvm/build_dict.rs

@@ -844,3 +844,17 @@ fn dict_symbol_to_zig_dict<'a, 'ctx, 'env>(
     )
     .into_struct_value()
 }
+
+pub fn decref<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    wrapper_struct: StructValue<'ctx>,
+    alignment: u32,
+) {
+    let pointer = env
+        .builder
+        .build_extract_value(wrapper_struct, Builtin::WRAPPER_PTR, "read_list_ptr")
+        .unwrap()
+        .into_pointer_value();
+
+    crate::llvm::refcounting::decref_pointer_check_null(env, pointer, alignment);
+}

compiler/gen_llvm/src/llvm/build_list.rs

@@ -1136,3 +1136,17 @@ pub fn store_list<'a, 'ctx, 'env>(
         "cast_collection",
     )
 }
+
+pub fn decref<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    wrapper_struct: StructValue<'ctx>,
+    alignment: u32,
+) {
+    let (_, pointer) = load_list(
+        env.builder,
+        wrapper_struct,
+        env.context.i8_type().ptr_type(AddressSpace::Generic),
+    );
+
+    crate::llvm::refcounting::decref_pointer_check_null(env, pointer, alignment);
+}

compiler/gen_llvm/src/llvm/refcounting.rs

@@ -84,7 +84,7 @@ impl<'ctx> PointerToRefcount<'ctx> {
         }
     }
 
-    pub fn from_list_wrapper(env: &Env<'_, 'ctx, '_>, list_wrapper: StructValue<'ctx>) -> Self {
+    fn from_list_wrapper(env: &Env<'_, 'ctx, '_>, list_wrapper: StructValue<'ctx>) -> Self {
         let data_ptr = env
             .builder
             .build_extract_value(list_wrapper, Builtin::WRAPPER_PTR, "read_list_ptr")
@@ -102,7 +102,7 @@ impl<'ctx> PointerToRefcount<'ctx> {
             .build_int_compare(IntPredicate::EQ, current, one, "is_one")
     }
 
-    pub fn get_refcount<'a, 'env>(&self, env: &Env<'a, 'ctx, 'env>) -> IntValue<'ctx> {
+    fn get_refcount<'a, 'env>(&self, env: &Env<'a, 'ctx, 'env>) -> IntValue<'ctx> {
         env.builder
             .build_load(self.value, "get_refcount")
             .into_int_value()
@@ -220,25 +220,57 @@ impl<'ctx> PointerToRefcount<'ctx> {
 
         debug_info_init!(env, parent);
 
-        let alignment = env.context.i32_type().const_int(alignment as _, false);
-
-        call_void_bitcode_fn(
+        decref_pointer(
             env,
-            &[
-                env.builder.build_bitcast(
-                    parent.get_nth_param(0).unwrap(),
-                    env.ptr_int().ptr_type(AddressSpace::Generic),
-                    "foo",
-                ),
-                alignment.into(),
-            ],
-            roc_builtins::bitcode::UTILS_DECREF,
+            parent.get_nth_param(0).unwrap().into_pointer_value(),
+            alignment,
         );
 
         builder.build_return(None);
     }
 }
 
+fn decref_pointer<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    pointer: PointerValue<'ctx>,
+    alignment: u32,
+) {
+    let alignment = env.context.i32_type().const_int(alignment as _, false);
+    call_void_bitcode_fn(
+        env,
+        &[
+            env.builder.build_bitcast(
+                pointer,
+                env.ptr_int().ptr_type(AddressSpace::Generic),
+                "to_isize_ptr",
+            ),
+            alignment.into(),
+        ],
+        roc_builtins::bitcode::UTILS_DECREF,
+    );
+}
+
+/// Assumes a pointer to the refcount
+pub fn decref_pointer_check_null<'a, 'ctx, 'env>(
+    env: &Env<'a, 'ctx, 'env>,
+    pointer: PointerValue<'ctx>,
+    alignment: u32,
+) {
+    let alignment = env.context.i32_type().const_int(alignment as _, false);
+    call_void_bitcode_fn(
+        env,
+        &[
+            env.builder.build_bitcast(
+                pointer,
+                env.context.i8_type().ptr_type(AddressSpace::Generic),
+                "to_i8_ptr",
+            ),
+            alignment.into(),
+        ],
+        roc_builtins::bitcode::UTILS_DECREF_CHECK_NULL,
+    );
+}
+
 fn modify_refcount_struct<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
     layout_ids: &mut LayoutIds<'a>,
@@ -1093,7 +1125,6 @@ fn build_rec_union_help<'a, 'ctx, 'env>(
     fn_val: FunctionValue<'ctx>,
 ) {
     let tags = union_layout_tags(env.arena, &union_layout);
-    let is_nullable = union_layout.is_nullable();
     debug_assert!(!tags.is_empty());
 
     let context = &env.context;
@@ -1126,7 +1157,7 @@ fn build_rec_union_help<'a, 'ctx, 'env>(
     let should_recurse_block = env.context.append_basic_block(parent, "should_recurse");
 
     let ctx = env.context;
-    if is_nullable {
+    if union_layout.is_nullable() {
         let is_null = env.builder.build_is_null(value_ptr, "is_null");
 
         let then_block = ctx.append_basic_block(parent, "then");
@@ -1201,6 +1232,12 @@ enum DecOrReuse {
     Reuse,
 }
 
+fn fields_need_no_refcounting(field_layouts: &[Layout]) -> bool {
+    !field_layouts
+        .iter()
+        .any(|x| x.is_refcounted() || x.contains_refcounted())
+}
+
 #[allow(clippy::too_many_arguments)]
 fn build_rec_union_recursive_decrement<'a, 'ctx, 'env>(
     env: &Env<'a, 'ctx, 'env>,
@@ -1220,21 +1257,6 @@ fn build_rec_union_recursive_decrement<'a, 'ctx, 'env>(
     let call_mode = mode_to_call_mode(decrement_fn, mode);
     let builder = env.builder;
 
-    // branches that are not/don't contain anything refcounted
-    // if there is only one branch, we don't need to switch
-    let switch_needed: bool = (|| {
-        for field_layouts in tags.iter() {
-            // if none of the fields are or contain anything refcounted, just move on
-            if !field_layouts
-                .iter()
-                .any(|x| x.is_refcounted() || x.contains_refcounted())
-            {
-                return true;
-            }
-        }
-        false
-    })();
-
     // next, make a jump table for all possible values of the tag_id
     let mut cases = Vec::with_capacity_in(tags.len(), env.arena);
 
@@ -1243,10 +1265,7 @@ fn build_rec_union_recursive_decrement<'a, 'ctx, 'env>(
 
     for (tag_id, field_layouts) in tags.iter().enumerate() {
         // if none of the fields are or contain anything refcounted, just move on
-        if !field_layouts
-            .iter()
-            .any(|x| x.is_refcounted() || x.contains_refcounted())
-        {
+        if fields_need_no_refcounting(field_layouts) {
             continue;
         }
 
@@ -1346,11 +1365,13 @@ fn build_rec_union_recursive_decrement<'a, 'ctx, 'env>(
 
     cases.reverse();
 
-    if cases.len() == 1 && !switch_needed {
-        // there is only one tag in total; we don't need a switch
-        // this is essential for nullable unwrapped layouts,
-        // because the `else` branch below would try to read its
-        // (nonexistant) tag id
+    if matches!(
+        union_layout,
+        UnionLayout::NullableUnwrapped { .. } | UnionLayout::NonNullableUnwrapped { .. }
+    ) {
+        debug_assert_eq!(cases.len(), 1);
+
+        // in this case, don't switch, because the `else` branch below would try to read the (nonexistant) tag id
         let (_, only_branch) = cases.pop().unwrap();
         env.builder.build_unconditional_branch(only_branch);
     } else {
@@ -1444,7 +1465,6 @@ fn build_reuse_rec_union_help<'a, 'ctx, 'env>(
     dec_function: FunctionValue<'ctx>,
 ) {
     let tags = union_layout_tags(env.arena, &union_layout);
-    let is_nullable = union_layout.is_nullable();
 
     debug_assert!(!tags.is_empty());
 
@@ -1478,7 +1498,7 @@ fn build_reuse_rec_union_help<'a, 'ctx, 'env>(
     let should_recurse_block = env.context.append_basic_block(parent, "should_recurse");
 
     let ctx = env.context;
-    if is_nullable {
+    if union_layout.is_nullable() {
         let is_null = env.builder.build_is_null(value_ptr, "is_null");
 
         let then_block = ctx.append_basic_block(parent, "then");
@@ -1700,68 +1720,3 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>(
     // this function returns void
     builder.build_return(None);
 }
-
-pub fn refcount_is_one_comparison<'a, 'ctx, 'env>(
-    env: &Env<'a, 'ctx, 'env>,
-    refcount: IntValue<'ctx>,
-) -> IntValue<'ctx> {
-    env.builder.build_int_compare(
-        IntPredicate::EQ,
-        refcount,
-        refcount_1(env.context, env.ptr_bytes),
-        "refcount_one_check",
-    )
-}
-
-pub fn list_get_refcount_ptr<'a, 'ctx, 'env>(
-    env: &Env<'a, 'ctx, 'env>,
-    layout: &Layout<'a>,
-    list_wrapper: StructValue<'ctx>,
-) -> PointerValue<'ctx> {
-    // fetch the pointer to the array data, as an integer
-    let ptr_as_int = env
-        .builder
-        .build_extract_value(list_wrapper, Builtin::WRAPPER_PTR, "read_list_ptr")
-        .unwrap()
-        .into_int_value();
-
-    get_refcount_ptr_help(env, layout, ptr_as_int)
-}
-
-pub fn refcount_offset<'a, 'ctx, 'env>(env: &Env<'a, 'ctx, 'env>, layout: &Layout<'a>) -> u64 {
-    let value_bytes = layout.stack_size(env.ptr_bytes) as u64;
-
-    match layout {
-        Layout::Builtin(Builtin::List(_)) => env.ptr_bytes as u64,
-        Layout::Builtin(Builtin::Str) => env.ptr_bytes as u64,
-        Layout::RecursivePointer | Layout::Union(_) => env.ptr_bytes as u64,
-        _ => (env.ptr_bytes as u64).max(value_bytes),
-    }
-}
-
-fn get_refcount_ptr_help<'a, 'ctx, 'env>(
-    env: &Env<'a, 'ctx, 'env>,
-    layout: &Layout<'a>,
-    ptr_as_int: IntValue<'ctx>,
-) -> PointerValue<'ctx> {
-    let builder = env.builder;
-    let ctx = env.context;
-
-    let offset = refcount_offset(env, layout);
-
-    // pointer to usize
-    let refcount_type = ptr_int(ctx, env.ptr_bytes);
-
-    // subtract offset, to access the refcount
-    let refcount_ptr = builder.build_int_sub(
-        ptr_as_int,
-        refcount_type.const_int(offset, false),
-        "make_refcount_ptr",
-    );
-
-    builder.build_int_to_ptr(
-        refcount_ptr,
-        refcount_type.ptr_type(AddressSpace::Generic),
-        "get_refcount_ptr",
-    )
-}