From ba2e8cd32b0379e16efeebd6c8be1e5fd83ad70f Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 17:58:56 -0800 Subject: [PATCH 01/17] Add base piping for list.Replace --- compiler/builtins/bitcode/README.md | 2 +- compiler/builtins/bitcode/src/list.zig | 87 ++++++++++++++++++++++ compiler/builtins/bitcode/src/main.zig | 2 + compiler/builtins/src/bitcode.rs | 2 + compiler/builtins/src/std.rs | 18 ++++- compiler/can/src/builtins.rs | 94 +++++++++++++++++++++++- compiler/gen_llvm/src/llvm/build.rs | 19 ++++- compiler/gen_llvm/src/llvm/build_list.rs | 44 +++++++++++ compiler/gen_wasm/src/low_level.rs | 16 ++-- compiler/module/src/low_level.rs | 2 + compiler/module/src/symbol.rs | 1 + compiler/mono/src/borrow.rs | 1 + compiler/test_gen/src/gen_list.rs | 34 +++++++++ compiler/types/src/builtin_aliases.rs | 9 +++ 14 files changed, 315 insertions(+), 16 deletions(-) diff --git a/compiler/builtins/bitcode/README.md b/compiler/builtins/bitcode/README.md index 604f3a53d0..1585c2e8fb 100644 --- a/compiler/builtins/bitcode/README.md +++ b/compiler/builtins/bitcode/README.md @@ -7,7 +7,7 @@ To add a builtin: 2. Make sure the function is public with the `pub` keyword and uses the C calling convention. This is really easy, just add `pub` and `callconv(.C)` to the function declaration like so: `pub fn atan(num: f64) callconv(.C) f64 { ... }` 3. In `src/main.zig`, export the function. This is also organized by module. For example, for a `Num` function find the `Num` section and add: `comptime { exportNumFn(num.atan, "atan"); }`. The first argument is the function, the second is the name of it in LLVM. 4. In `compiler/builtins/src/bitcode.rs`, add a constant for the new function. This is how we use it in Rust. Once again, this is organized by module, so just find the relevant area and add your new function. -5. You can now your function in Rust using `call_bitcode_fn` in `llvm/src/build.rs`! +5. You can now use your function in Rust using `call_bitcode_fn` in `llvm/src/build.rs`! ## How it works diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 70925554f1..012f24b141 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -1256,6 +1256,93 @@ pub fn listConcat(list_a: RocList, list_b: RocList, alignment: u32, element_widt return output; } +pub fn listReplaceInPlace( + bytes: ?[*]u8, + index: usize, + element: Opaque, + element_width: usize, +) callconv(.C) ?[*]u8 { + // INVARIANT: bounds checking happens on the roc side + // + // at the time of writing, the function is implemented roughly as + // `if inBounds then LowLevelListReplace input index item else input` + // so we don't do a bounds check here. Hence, the list is also non-empty, + // because inserting into an empty list is always out of bounds + + return listReplaceInPlaceHelp(bytes, index, element, element_width); +} + +pub fn listReplace( + bytes: ?[*]u8, + length: usize, + alignment: u32, + index: usize, + element: Opaque, + element_width: usize, +) callconv(.C) ?[*]u8 { + // INVARIANT: bounds checking happens on the roc side + // + // at the time of writing, the function is implemented roughly as + // `if inBounds then LowLevelListReplace input index item else input` + // so we don't do a bounds check here. Hence, the list is also non-empty, + // because inserting into an empty list is always out of bounds + const ptr: [*]usize = @ptrCast([*]usize, @alignCast(@alignOf(usize), bytes)); + + if ((ptr - 1)[0] == utils.REFCOUNT_ONE) { + return listReplaceInPlaceHelp(bytes, index, element, element_width); + } else { + return listReplaceImmutable(bytes, length, alignment, index, element, element_width); + } +} + +inline fn listReplaceInPlaceHelp( + bytes: ?[*]u8, + index: usize, + element: Opaque, + element_width: usize, +) ?[*]u8 { + // the element we will replace + var element_at_index = (bytes orelse undefined) + (index * element_width); + + // decrement its refcount + // dec(element_at_index); + + // copy in the new element + @memcpy(element_at_index, element orelse undefined, element_width); + + return bytes; +} + +inline fn listReplaceImmutable( + old_bytes: ?[*]u8, + length: usize, + alignment: u32, + index: usize, + element: Opaque, + element_width: usize, +) ?[*]u8 { + const data_bytes = length * element_width; + + var new_bytes = utils.allocateWithRefcount(data_bytes, alignment); + + @memcpy(new_bytes, old_bytes orelse undefined, data_bytes); + + // the element we will replace + var element_at_index = new_bytes + (index * element_width); + + // decrement its refcount + // dec(element_at_index); + + // copy in the new element + @memcpy(element_at_index, element orelse undefined, element_width); + + // consume RC token of original + utils.decref(old_bytes, data_bytes, alignment); + + //return list; + return new_bytes; +} + pub fn listSetInPlace( bytes: ?[*]u8, index: usize, diff --git a/compiler/builtins/bitcode/src/main.zig b/compiler/builtins/bitcode/src/main.zig index c047588d92..8a23f09985 100644 --- a/compiler/builtins/bitcode/src/main.zig +++ b/compiler/builtins/bitcode/src/main.zig @@ -49,6 +49,8 @@ comptime { exportListFn(list.listConcat, "concat"); exportListFn(list.listSublist, "sublist"); exportListFn(list.listDropAt, "drop_at"); + exportListFn(list.listReplace, "replace"); + exportListFn(list.listReplaceInPlace, "replace_in_place"); exportListFn(list.listSet, "set"); exportListFn(list.listSetInPlace, "set_in_place"); exportListFn(list.listSwap, "swap"); diff --git a/compiler/builtins/src/bitcode.rs b/compiler/builtins/src/bitcode.rs index 1345156d0f..c4c0b12e01 100644 --- a/compiler/builtins/src/bitcode.rs +++ b/compiler/builtins/src/bitcode.rs @@ -354,6 +354,8 @@ pub const LIST_RANGE: &str = "roc_builtins.list.range"; pub const LIST_REVERSE: &str = "roc_builtins.list.reverse"; pub const LIST_SORT_WITH: &str = "roc_builtins.list.sort_with"; pub const LIST_CONCAT: &str = "roc_builtins.list.concat"; +pub const LIST_REPLACE: &str = "roc_builtins.list.replace"; +pub const LIST_REPLACE_IN_PLACE: &str = "roc_builtins.list.replace_in_place"; pub const LIST_SET: &str = "roc_builtins.list.set"; pub const LIST_SET_IN_PLACE: &str = "roc_builtins.list.set_in_place"; pub const LIST_ANY: &str = "roc_builtins.list.any"; diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index 97394b969b..35ab535a6a 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -4,9 +4,9 @@ use roc_module::symbol::Symbol; use roc_region::all::Region; use roc_types::builtin_aliases::{ bool_type, dec_type, dict_type, f32_type, f64_type, float_type, i128_type, i16_type, i32_type, - i64_type, i8_type, int_type, list_type, nat_type, num_type, ordering_type, result_type, - set_type, str_type, str_utf8_byte_problem_type, u128_type, u16_type, u32_type, u64_type, - u8_type, + i64_type, i8_type, int_type, list_type, nat_type, num_type, ordering_type, pair_type, + result_type, set_type, str_type, str_utf8_byte_problem_type, u128_type, u16_type, u32_type, + u64_type, u8_type, }; use roc_types::solved_types::SolvedType; use roc_types::subs::VarId; @@ -1034,7 +1034,7 @@ pub fn types() -> MutMap { add_top_level_function_type!( Symbol::LIST_GET, vec![list_type(flex(TVAR1)), nat_type()], - Box::new(result_type(flex(TVAR1), index_out_of_bounds)), + Box::new(result_type(flex(TVAR1), index_out_of_bounds.clone())), ); // first : List elem -> Result elem [ ListWasEmpty ]* @@ -1056,6 +1056,16 @@ pub fn types() -> MutMap { Box::new(result_type(flex(TVAR1), list_was_empty.clone())), ); + // replace : List elem, Nat, elem -> Result (Pair (List elem) elem) [ OutOfBounds ]* + add_top_level_function_type!( + Symbol::LIST_REPLACE, + vec![list_type(flex(TVAR1)), nat_type(), flex(TVAR1)], + Box::new(result_type( + pair_type(list_type(flex(TVAR1)), flex(TVAR1)), + index_out_of_bounds + )), + ); + // set : List elem, Nat, elem -> List elem add_top_level_function_type!( Symbol::LIST_SET, diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index a8228c6ecb..ae6375ada7 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -102,6 +102,7 @@ pub fn builtin_defs_map(symbol: Symbol, var_store: &mut VarStore) -> Option STR_TO_I8 => str_to_num, LIST_LEN => list_len, LIST_GET => list_get, + LIST_REPLACE => list_replace, LIST_SET => list_set, LIST_APPEND => list_append, LIST_FIRST => list_first, @@ -2303,6 +2304,97 @@ fn list_get(symbol: Symbol, var_store: &mut VarStore) -> Def { ) } +/// List.replace : List elem, Nat, elem -> Result (Pair (List elem) elem) [ OutOfBounds ]* +/// +/// List.replace : +/// Attr (w | u | v) (List (Attr u a)), +/// Attr * Int, +/// Attr (u | v) a +/// -> Attr * (List (Attr u a)) +/// -> Attr * (Result (Pair (List (Attr u a)) Attr u a)) (Attr * [ OutOfBounds ]*)) +fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { + let arg_list = Symbol::ARG_1; + let arg_index = Symbol::ARG_2; + let arg_elem = Symbol::ARG_3; + let bool_var = var_store.fresh(); + let len_var = var_store.fresh(); + let elem_var = var_store.fresh(); + let list_arg_var = var_store.fresh(); + let ret_pair_var = var_store.fresh(); + + // Perform a bounds check. If it passes, run LowLevel::ListReplace. + // Otherwise, return the list unmodified. + let body = If { + cond_var: bool_var, + branch_var: ret_pair_var, + branches: vec![( + // if-condition + no_region( + // index < List.len list + RunLowLevel { + op: LowLevel::NumLt, + args: vec![ + (len_var, Var(arg_index)), + ( + len_var, + RunLowLevel { + op: LowLevel::ListLen, + args: vec![(list_arg_var, Var(arg_list))], + ret_var: len_var, + }, + ), + ], + ret_var: bool_var, + }, + ), + // then-branch + no_region( + // Ok + tag( + "Ok", + vec![ + // TODO: This should probably call get and then build the pair + // List.replaceUnsafe list index elem + RunLowLevel { + op: LowLevel::ListReplace, + args: vec![ + (list_arg_var, Var(arg_list)), + (len_var, Var(arg_index)), + (elem_var, Var(arg_elem)), + ], + ret_var: ret_pair_var, + }, + ], + var_store, + ), + ), + )], + final_else: Box::new( + // else-branch + no_region( + // Err + tag( + "Err", + vec![tag("OutOfBounds", Vec::new(), var_store)], + var_store, + ), + ), + ), + }; + + defn( + symbol, + vec![ + (list_arg_var, Symbol::ARG_1), + (len_var, Symbol::ARG_2), + (elem_var, Symbol::ARG_3), + ], + var_store, + body, + ret_pair_var, + ) +} + /// List.set : List elem, Nat, elem -> List elem /// /// List.set : @@ -2347,7 +2439,7 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { ), // then-branch no_region( - // List.setUnsafe list index + // List.setUnsafe list index elem RunLowLevel { op: LowLevel::ListSet, args: vec![ diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index af42090989..b44f84767b 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -13,8 +13,8 @@ use crate::llvm::build_list::{ self, allocate_list, empty_polymorphic_list, list_all, list_any, list_append, list_concat, list_contains, list_drop_at, list_find_unsafe, list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len, list_map, list_map2, list_map3, list_map4, - list_map_with_index, list_prepend, list_range, list_repeat, list_reverse, list_set, - list_single, list_sort_with, list_sublist, list_swap, + list_map_with_index, list_prepend, list_range, list_repeat, list_replace, list_reverse, + list_set, list_single, list_sort_with, list_sublist, list_swap, }; use crate::llvm::build_str::{ str_concat, str_count_graphemes, str_ends_with, str_from_float, str_from_int, str_from_utf8, @@ -5653,6 +5653,21 @@ fn run_low_level<'a, 'ctx, 'env>( wrapper_struct, ) } + ListReplace => { + let list = load_symbol(scope, &args[0]); + let index = load_symbol(scope, &args[1]); + let (element, element_layout) = load_symbol_and_layout(scope, &args[2]); + + list_replace( + env, + layout_ids, + list, + index.into_int_value(), + element, + element_layout, + update_mode, + ) + } ListSet => { let list = load_symbol(scope, &args[0]); let index = load_symbol(scope, &args[1]); diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index 3f865b77aa..b27316a443 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -291,6 +291,50 @@ pub fn list_drop_at<'a, 'ctx, 'env>( ) } +/// List.replace : List elem, Nat, elem -> List elem +pub fn list_replace<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + layout_ids: &mut LayoutIds<'a>, + list: BasicValueEnum<'ctx>, + index: IntValue<'ctx>, + element: BasicValueEnum<'ctx>, + element_layout: &Layout<'a>, + update_mode: UpdateMode, +) -> BasicValueEnum<'ctx> { + let (length, bytes) = load_list( + env.builder, + list.into_struct_value(), + env.context.i8_type().ptr_type(AddressSpace::Generic), + ); + + let new_bytes = match update_mode { + UpdateMode::InPlace => call_bitcode_fn( + env, + &[ + bytes.into(), + index.into(), + pass_element_as_opaque(env, element, *element_layout), + layout_width(env, element_layout), + ], + bitcode::LIST_REPLACE_IN_PLACE, + ), + UpdateMode::Immutable => call_bitcode_fn( + env, + &[ + bytes.into(), + length.into(), + env.alignment_intvalue(element_layout), + index.into(), + pass_element_as_opaque(env, element, *element_layout), + layout_width(env, element_layout), + ], + bitcode::LIST_REPLACE, + ), + }; + + store_list(env, new_bytes.into_pointer_value(), length) +} + /// List.set : List elem, Nat, elem -> List elem pub fn list_set<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, diff --git a/compiler/gen_wasm/src/low_level.rs b/compiler/gen_wasm/src/low_level.rs index a5875e1813..8d71d3f484 100644 --- a/compiler/gen_wasm/src/low_level.rs +++ b/compiler/gen_wasm/src/low_level.rs @@ -260,14 +260,14 @@ impl<'a> LowLevelCall<'a> { _ => internal_error!("invalid storage for List"), }, - ListGetUnsafe | ListSet | ListSingle | ListRepeat | ListReverse | ListConcat - | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap - | ListMap2 | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf | ListWalk - | ListWalkUntil | ListWalkBackwards | ListKeepOks | ListKeepErrs | ListSortWith - | ListSublist | ListDropAt | ListSwap | ListAny | ListAll | ListFindUnsafe - | DictSize | DictEmpty | DictInsert | DictRemove | DictContains | DictGetUnsafe - | DictKeys | DictValues | DictUnion | DictIntersection | DictDifference | DictWalk - | SetFromList => { + ListGetUnsafe | ListReplace | ListSet | ListSingle | ListRepeat | ListReverse + | ListConcat | ListContains | ListAppend | ListPrepend | ListJoin | ListRange + | ListMap | ListMap2 | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf + | ListWalk | ListWalkUntil | ListWalkBackwards | ListKeepOks | ListKeepErrs + | ListSortWith | ListSublist | ListDropAt | ListSwap | ListAny | ListAll + | ListFindUnsafe | DictSize | DictEmpty | DictInsert | DictRemove | DictContains + | DictGetUnsafe | DictKeys | DictValues | DictUnion | DictIntersection + | DictDifference | DictWalk | SetFromList => { todo!("{:?}", self.lowlevel); } diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index be5a8394d3..720382ce1a 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -28,6 +28,7 @@ pub enum LowLevel { ListSet, ListSingle, ListRepeat, + ListReplace, ListReverse, ListConcat, ListContains, @@ -229,6 +230,7 @@ impl LowLevelWrapperType { Symbol::LIST_LEN => CanBeReplacedBy(ListLen), Symbol::LIST_GET => WrapperIsRequired, Symbol::LIST_SET => WrapperIsRequired, + Symbol::LIST_REPLACE => WrapperIsRequired, Symbol::LIST_SINGLE => CanBeReplacedBy(ListSingle), Symbol::LIST_REPEAT => CanBeReplacedBy(ListRepeat), Symbol::LIST_REVERSE => CanBeReplacedBy(ListReverse), diff --git a/compiler/module/src/symbol.rs b/compiler/module/src/symbol.rs index 82778fa6d0..80d4bd235c 100644 --- a/compiler/module/src/symbol.rs +++ b/compiler/module/src/symbol.rs @@ -1141,6 +1141,7 @@ define_builtins! { 55 LIST_SORT_ASC: "sortAsc" 56 LIST_SORT_DESC: "sortDesc" 57 LIST_SORT_DESC_COMPARE: "#sortDescCompare" + 58 LIST_REPLACE: "replace" } 5 RESULT: "Result" => { 0 RESULT_RESULT: "Result" imported // the Result.Result type alias diff --git a/compiler/mono/src/borrow.rs b/compiler/mono/src/borrow.rs index 3f6a0bbc4e..d8ed8fa610 100644 --- a/compiler/mono/src/borrow.rs +++ b/compiler/mono/src/borrow.rs @@ -935,6 +935,7 @@ pub fn lowlevel_borrow_signature(arena: &Bump, op: LowLevel) -> &[bool] { match op { ListLen | StrIsEmpty | StrCountGraphemes => arena.alloc_slice_copy(&[borrowed]), ListSet => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), + ListReplace => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), ListGetUnsafe => arena.alloc_slice_copy(&[borrowed, irrelevant]), ListConcat => arena.alloc_slice_copy(&[owned, owned]), StrConcat => arena.alloc_slice_copy(&[owned, borrowed]), diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index 333bdb4a63..ba8270808e 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1702,6 +1702,40 @@ fn get_int_list_oob() { ); } +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn replace_unique_int_list() { + assert_evals_to!( + indoc!( + r#" + result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 + when result is + Ok (Pair newList _) -> newList + Err _ -> [] + "# + ), + RocList::from_slice(&[12, 9, 33, 1, 5]), + RocList + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn replace_unique_int_list_get_old_value() { + assert_evals_to!( + indoc!( + r#" + result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 + when result is + Ok (Pair _ oldValue) -> oldValue + Err _ -> -1 + "# + ), + 7, + i64 + ); +} + #[test] #[cfg(any(feature = "gen-llvm"))] fn get_set_unique_int_list() { diff --git a/compiler/types/src/builtin_aliases.rs b/compiler/types/src/builtin_aliases.rs index f8df53a256..07efa07c9e 100644 --- a/compiler/types/src/builtin_aliases.rs +++ b/compiler/types/src/builtin_aliases.rs @@ -914,6 +914,15 @@ pub fn ordering_type() -> SolvedType { ) } +#[inline(always)] +pub fn pair_type(t1: SolvedType, t2: SolvedType) -> SolvedType { + // [ Pair t1 t2 ] + SolvedType::TagUnion( + vec![(TagName::Global("Pair".into()), vec![t1, t2])], + Box::new(SolvedType::EmptyTagUnion), + ) +} + #[inline(always)] pub fn result_type(a: SolvedType, e: SolvedType) -> SolvedType { SolvedType::Alias( From 27b47713aa5ceae1b110d55365b59fca556f0525 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 20:40:45 -0800 Subject: [PATCH 02/17] Add some comments and TODOs --- compiler/builtins/bitcode/src/list.zig | 8 ++++++++ compiler/gen_llvm/src/llvm/build_list.rs | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 012f24b141..7723a59668 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -1301,6 +1301,10 @@ inline fn listReplaceInPlaceHelp( element: Opaque, element_width: usize, ) ?[*]u8 { + // TODO: figure out how to return an element and a List. + // We only know the elment size at runtime. + // This code is currently the same as listSet. + // the element we will replace var element_at_index = (bytes orelse undefined) + (index * element_width); @@ -1321,6 +1325,10 @@ inline fn listReplaceImmutable( element: Opaque, element_width: usize, ) ?[*]u8 { + // TODO: figure out how to return an element and a List. + // We only know the elment size at runtime. + // This code is currently the same as listSet. + const data_bytes = length * element_width; var new_bytes = utils.allocateWithRefcount(data_bytes, alignment); diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index b27316a443..fa709380a5 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -294,19 +294,22 @@ pub fn list_drop_at<'a, 'ctx, 'env>( /// List.replace : List elem, Nat, elem -> List elem pub fn list_replace<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, - layout_ids: &mut LayoutIds<'a>, + _layout_ids: &mut LayoutIds<'a>, list: BasicValueEnum<'ctx>, index: IntValue<'ctx>, element: BasicValueEnum<'ctx>, element_layout: &Layout<'a>, update_mode: UpdateMode, ) -> BasicValueEnum<'ctx> { + // TODO: This or elsewhere needs to deal with building the record that gets returned. let (length, bytes) = load_list( env.builder, list.into_struct_value(), env.context.i8_type().ptr_type(AddressSpace::Generic), ); + // Assume the bounds have already been checked earlier + // (e.g. by List.replace or List.set, which wrap List.#replaceUnsafe) let new_bytes = match update_mode { UpdateMode::InPlace => call_bitcode_fn( env, From dddf8ff785e9ff997d5d0fdaa7a32024aa8e1cd3 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 20:41:26 -0800 Subject: [PATCH 03/17] switch from pair to record and change name to ListReplaceUnsafe --- compiler/builtins/src/std.rs | 16 +++++++++++----- compiler/can/src/builtins.rs | 18 ++++++++---------- compiler/gen_llvm/src/llvm/build.rs | 6 +++--- compiler/gen_llvm/src/llvm/build_list.rs | 4 ++-- compiler/gen_wasm/src/low_level.rs | 2 +- compiler/module/src/low_level.rs | 2 +- compiler/mono/src/borrow.rs | 2 +- compiler/test_gen/src/gen_list.rs | 4 ++-- 8 files changed, 29 insertions(+), 25 deletions(-) diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index 35ab535a6a..0ec5e0e28a 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -4,9 +4,9 @@ use roc_module::symbol::Symbol; use roc_region::all::Region; use roc_types::builtin_aliases::{ bool_type, dec_type, dict_type, f32_type, f64_type, float_type, i128_type, i16_type, i32_type, - i64_type, i8_type, int_type, list_type, nat_type, num_type, ordering_type, pair_type, - result_type, set_type, str_type, str_utf8_byte_problem_type, u128_type, u16_type, u32_type, - u64_type, u8_type, + i64_type, i8_type, int_type, list_type, nat_type, num_type, ordering_type, result_type, + set_type, str_type, str_utf8_byte_problem_type, u128_type, u16_type, u32_type, u64_type, + u8_type, }; use roc_types::solved_types::SolvedType; use roc_types::subs::VarId; @@ -1056,12 +1056,18 @@ pub fn types() -> MutMap { Box::new(result_type(flex(TVAR1), list_was_empty.clone())), ); - // replace : List elem, Nat, elem -> Result (Pair (List elem) elem) [ OutOfBounds ]* + // replace : List elem, Nat, elem -> Result { list: List elem, value: elem } [ OutOfBounds ]* add_top_level_function_type!( Symbol::LIST_REPLACE, vec![list_type(flex(TVAR1)), nat_type(), flex(TVAR1)], Box::new(result_type( - pair_type(list_type(flex(TVAR1)), flex(TVAR1)), + SolvedType::Record { + fields: vec![ + ("list".into(), RecordField::Required(list_type(flex(TVAR1)))), + ("value".into(), RecordField::Required(flex(TVAR1))), + ], + ext: Box::new(SolvedType::EmptyRecord), + }, index_out_of_bounds )), ); diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index ae6375ada7..ee035c30cd 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -2304,14 +2304,13 @@ fn list_get(symbol: Symbol, var_store: &mut VarStore) -> Def { ) } -/// List.replace : List elem, Nat, elem -> Result (Pair (List elem) elem) [ OutOfBounds ]* +/// List.replace : List elem, Nat, elem -> Result { list: List elem, value: elem } [ OutOfBounds ]* /// /// List.replace : /// Attr (w | u | v) (List (Attr u a)), /// Attr * Int, /// Attr (u | v) a -/// -> Attr * (List (Attr u a)) -/// -> Attr * (Result (Pair (List (Attr u a)) Attr u a)) (Attr * [ OutOfBounds ]*)) +/// -> Attr * (Result { list: List (Attr u a), value: Attr u a } (Attr * [ OutOfBounds ]*)) fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { let arg_list = Symbol::ARG_1; let arg_index = Symbol::ARG_2; @@ -2320,13 +2319,13 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { let len_var = var_store.fresh(); let elem_var = var_store.fresh(); let list_arg_var = var_store.fresh(); - let ret_pair_var = var_store.fresh(); + let ret_record_var = var_store.fresh(); - // Perform a bounds check. If it passes, run LowLevel::ListReplace. + // Perform a bounds check. If it passes, run LowLevel::ListReplaceUnsafe. // Otherwise, return the list unmodified. let body = If { cond_var: bool_var, - branch_var: ret_pair_var, + branch_var: ret_record_var, branches: vec![( // if-condition no_region( @@ -2353,16 +2352,15 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { tag( "Ok", vec![ - // TODO: This should probably call get and then build the pair // List.replaceUnsafe list index elem RunLowLevel { - op: LowLevel::ListReplace, + op: LowLevel::ListReplaceUnsafe, args: vec![ (list_arg_var, Var(arg_list)), (len_var, Var(arg_index)), (elem_var, Var(arg_elem)), ], - ret_var: ret_pair_var, + ret_var: ret_record_var, }, ], var_store, @@ -2391,7 +2389,7 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { ], var_store, body, - ret_pair_var, + ret_record_var, ) } diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index b44f84767b..347bd2d044 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -13,7 +13,7 @@ use crate::llvm::build_list::{ self, allocate_list, empty_polymorphic_list, list_all, list_any, list_append, list_concat, list_contains, list_drop_at, list_find_unsafe, list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len, list_map, list_map2, list_map3, list_map4, - list_map_with_index, list_prepend, list_range, list_repeat, list_replace, list_reverse, + list_map_with_index, list_prepend, list_range, list_repeat, list_replace_unsafe, list_reverse, list_set, list_single, list_sort_with, list_sublist, list_swap, }; use crate::llvm::build_str::{ @@ -5653,12 +5653,12 @@ fn run_low_level<'a, 'ctx, 'env>( wrapper_struct, ) } - ListReplace => { + ListReplaceUnsafe => { let list = load_symbol(scope, &args[0]); let index = load_symbol(scope, &args[1]); let (element, element_layout) = load_symbol_and_layout(scope, &args[2]); - list_replace( + list_replace_unsafe( env, layout_ids, list, diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index fa709380a5..fd528bbac1 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -291,8 +291,8 @@ pub fn list_drop_at<'a, 'ctx, 'env>( ) } -/// List.replace : List elem, Nat, elem -> List elem -pub fn list_replace<'a, 'ctx, 'env>( +/// List.replace_unsafe : List elem, Nat, elem -> { list: List elem, value: elem } +pub fn list_replace_unsafe<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, _layout_ids: &mut LayoutIds<'a>, list: BasicValueEnum<'ctx>, diff --git a/compiler/gen_wasm/src/low_level.rs b/compiler/gen_wasm/src/low_level.rs index 8d71d3f484..b68d8e968a 100644 --- a/compiler/gen_wasm/src/low_level.rs +++ b/compiler/gen_wasm/src/low_level.rs @@ -260,7 +260,7 @@ impl<'a> LowLevelCall<'a> { _ => internal_error!("invalid storage for List"), }, - ListGetUnsafe | ListReplace | ListSet | ListSingle | ListRepeat | ListReverse + ListGetUnsafe | ListReplaceUnsafe | ListSet | ListSingle | ListRepeat | ListReverse | ListConcat | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap | ListMap2 | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf | ListWalk | ListWalkUntil | ListWalkBackwards | ListKeepOks | ListKeepErrs diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index 720382ce1a..ad10a1cbba 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -28,7 +28,7 @@ pub enum LowLevel { ListSet, ListSingle, ListRepeat, - ListReplace, + ListReplaceUnsafe, ListReverse, ListConcat, ListContains, diff --git a/compiler/mono/src/borrow.rs b/compiler/mono/src/borrow.rs index d8ed8fa610..d46ccbc8ee 100644 --- a/compiler/mono/src/borrow.rs +++ b/compiler/mono/src/borrow.rs @@ -935,7 +935,7 @@ pub fn lowlevel_borrow_signature(arena: &Bump, op: LowLevel) -> &[bool] { match op { ListLen | StrIsEmpty | StrCountGraphemes => arena.alloc_slice_copy(&[borrowed]), ListSet => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), - ListReplace => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), + ListReplaceUnsafe => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), ListGetUnsafe => arena.alloc_slice_copy(&[borrowed, irrelevant]), ListConcat => arena.alloc_slice_copy(&[owned, owned]), StrConcat => arena.alloc_slice_copy(&[owned, borrowed]), diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index ba8270808e..296f083e59 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1710,7 +1710,7 @@ fn replace_unique_int_list() { r#" result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 when result is - Ok (Pair newList _) -> newList + Ok {list, value} -> list Err _ -> [] "# ), @@ -1727,7 +1727,7 @@ fn replace_unique_int_list_get_old_value() { r#" result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 when result is - Ok (Pair _ oldValue) -> oldValue + Ok {list, value} -> value Err _ -> -1 "# ), From dc59ba97c2eed6177f19ebf5f6ddb8c9a3d49270 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:02:23 -0800 Subject: [PATCH 04/17] change listReplace zig builtin to use RocList --- compiler/builtins/bitcode/src/list.zig | 73 ++++++-------------------- 1 file changed, 15 insertions(+), 58 deletions(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 7723a59668..6ad31ced7c 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -1257,98 +1257,55 @@ pub fn listConcat(list_a: RocList, list_b: RocList, alignment: u32, element_widt } pub fn listReplaceInPlace( - bytes: ?[*]u8, + list: RocList index: usize, element: Opaque, element_width: usize, -) callconv(.C) ?[*]u8 { + out_element: ?[*]u8, +) callconv(.C) extern RocList { // INVARIANT: bounds checking happens on the roc side // // at the time of writing, the function is implemented roughly as // `if inBounds then LowLevelListReplace input index item else input` // so we don't do a bounds check here. Hence, the list is also non-empty, // because inserting into an empty list is always out of bounds - - return listReplaceInPlaceHelp(bytes, index, element, element_width); + return listReplaceInPlaceHelp(list, index, element, element_width, out_element); } pub fn listReplace( - bytes: ?[*]u8, - length: usize, + list: RocList alignment: u32, index: usize, element: Opaque, element_width: usize, -) callconv(.C) ?[*]u8 { + out_element: ?[*]u8, +) callconv(.C) RocList { // INVARIANT: bounds checking happens on the roc side // // at the time of writing, the function is implemented roughly as // `if inBounds then LowLevelListReplace input index item else input` // so we don't do a bounds check here. Hence, the list is also non-empty, // because inserting into an empty list is always out of bounds - const ptr: [*]usize = @ptrCast([*]usize, @alignCast(@alignOf(usize), bytes)); - - if ((ptr - 1)[0] == utils.REFCOUNT_ONE) { - return listReplaceInPlaceHelp(bytes, index, element, element_width); - } else { - return listReplaceImmutable(bytes, length, alignment, index, element, element_width); - } + listReplaceInPlaceHelp(list.makeUnique(alignment, element_width), index, element, element_width, out_element); } inline fn listReplaceInPlaceHelp( - bytes: ?[*]u8, + list: RocList index: usize, element: Opaque, element_width: usize, -) ?[*]u8 { - // TODO: figure out how to return an element and a List. - // We only know the elment size at runtime. - // This code is currently the same as listSet. - + out_element: ?[*]u8, +) extern struct RocList { // the element we will replace - var element_at_index = (bytes orelse undefined) + (index * element_width); + var element_at_index = (list.bytes orelse undefined) + (index * element_width); - // decrement its refcount - // dec(element_at_index); + // copy out the old element + @memcpy(out_element orelse undefined, element_at_index, element_width); // copy in the new element @memcpy(element_at_index, element orelse undefined, element_width); - return bytes; -} - -inline fn listReplaceImmutable( - old_bytes: ?[*]u8, - length: usize, - alignment: u32, - index: usize, - element: Opaque, - element_width: usize, -) ?[*]u8 { - // TODO: figure out how to return an element and a List. - // We only know the elment size at runtime. - // This code is currently the same as listSet. - - const data_bytes = length * element_width; - - var new_bytes = utils.allocateWithRefcount(data_bytes, alignment); - - @memcpy(new_bytes, old_bytes orelse undefined, data_bytes); - - // the element we will replace - var element_at_index = new_bytes + (index * element_width); - - // decrement its refcount - // dec(element_at_index); - - // copy in the new element - @memcpy(element_at_index, element orelse undefined, element_width); - - // consume RC token of original - utils.decref(old_bytes, data_bytes, alignment); - - //return list; - return new_bytes; + return list; } pub fn listSetInPlace( From aff962809b2bd180228ee31a392c8f1d3c5065a6 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:33:12 -0800 Subject: [PATCH 05/17] call ListReplace and generate struct afterwards --- compiler/builtins/bitcode/src/list.zig | 12 +++---- compiler/gen_llvm/src/llvm/build_list.rs | 45 ++++++++++++++++++------ 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 6ad31ced7c..299f4f1591 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -1257,12 +1257,12 @@ pub fn listConcat(list_a: RocList, list_b: RocList, alignment: u32, element_widt } pub fn listReplaceInPlace( - list: RocList + list: RocList, index: usize, element: Opaque, element_width: usize, out_element: ?[*]u8, -) callconv(.C) extern RocList { +) callconv(.C) RocList { // INVARIANT: bounds checking happens on the roc side // // at the time of writing, the function is implemented roughly as @@ -1273,7 +1273,7 @@ pub fn listReplaceInPlace( } pub fn listReplace( - list: RocList + list: RocList, alignment: u32, index: usize, element: Opaque, @@ -1286,16 +1286,16 @@ pub fn listReplace( // `if inBounds then LowLevelListReplace input index item else input` // so we don't do a bounds check here. Hence, the list is also non-empty, // because inserting into an empty list is always out of bounds - listReplaceInPlaceHelp(list.makeUnique(alignment, element_width), index, element, element_width, out_element); + return listReplaceInPlaceHelp(list.makeUnique(alignment, element_width), index, element, element_width, out_element); } inline fn listReplaceInPlaceHelp( - list: RocList + list: RocList, index: usize, element: Opaque, element_width: usize, out_element: ?[*]u8, -) extern struct RocList { +) RocList { // the element we will replace var element_at_index = (list.bytes orelse undefined) + (index * element_width); diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index fd528bbac1..997e322411 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -301,41 +301,64 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( element_layout: &Layout<'a>, update_mode: UpdateMode, ) -> BasicValueEnum<'ctx> { - // TODO: This or elsewhere needs to deal with building the record that gets returned. - let (length, bytes) = load_list( - env.builder, - list.into_struct_value(), - env.context.i8_type().ptr_type(AddressSpace::Generic), - ); + let element_type = basic_type_from_layout(env, &element_layout); + let element_ptr = env + .builder + .build_alloca(element_type, "output_element_as_opaque"); // Assume the bounds have already been checked earlier // (e.g. by List.replace or List.set, which wrap List.#replaceUnsafe) - let new_bytes = match update_mode { + let new_list = match update_mode { UpdateMode::InPlace => call_bitcode_fn( env, &[ - bytes.into(), + list.into(), index.into(), pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), + pass_as_opaque(env, element_ptr), ], bitcode::LIST_REPLACE_IN_PLACE, ), UpdateMode::Immutable => call_bitcode_fn( env, &[ - bytes.into(), - length.into(), + list.into(), env.alignment_intvalue(element_layout), index.into(), pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), + pass_as_opaque(env, element_ptr), ], bitcode::LIST_REPLACE, ), }; - store_list(env, new_bytes.into_pointer_value(), length) + // Load the element and returned list into a struct. + let old_element = env.builder.build_load(element_ptr, "load_element"); + + let result = env + .context + .struct_type( + // TODO: does the order need to be decided by the size of the element type. + &[ + super::convert::zig_list_type(env).into(), + element_type.into(), + ], + false, + ) + .const_zero(); + + let result = env + .builder + .build_insert_value(result, old_element, 0, "insert_value") + .unwrap(); + + env.builder + .build_insert_value(result, new_list, 1, "insert_list") + .unwrap() + .into_struct_value() + .into() } /// List.set : List elem, Nat, elem -> List elem From edfbd6242f0e64aff6f93dbd6ce99a6d5a705f94 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:38:32 -0800 Subject: [PATCH 06/17] fix ListReplace generate if types --- compiler/can/src/builtins.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index ee035c30cd..550c7d0d0f 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -2320,12 +2320,13 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { let elem_var = var_store.fresh(); let list_arg_var = var_store.fresh(); let ret_record_var = var_store.fresh(); + let ret_result_var = var_store.fresh(); // Perform a bounds check. If it passes, run LowLevel::ListReplaceUnsafe. // Otherwise, return the list unmodified. let body = If { cond_var: bool_var, - branch_var: ret_record_var, + branch_var: var_store.fresh(), branches: vec![( // if-condition no_region( @@ -2389,7 +2390,7 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { ], var_store, body, - ret_record_var, + ret_result_var, ) } From 889b189191a0cc1b5ab1b88f63bf43a2a6caddaf Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:46:50 -0800 Subject: [PATCH 07/17] fix list passing --- compiler/gen_llvm/src/llvm/build_list.rs | 4 ++-- compiler/test_gen/src/gen_list.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index 997e322411..c6d39c33d5 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -312,7 +312,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( UpdateMode::InPlace => call_bitcode_fn( env, &[ - list.into(), + pass_list_cc(env, list), index.into(), pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), @@ -323,7 +323,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( UpdateMode::Immutable => call_bitcode_fn( env, &[ - list.into(), + pass_list_cc(env, list), env.alignment_intvalue(element_layout), index.into(), pass_element_as_opaque(env, element, *element_layout), diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index 296f083e59..18ce3051b5 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1710,7 +1710,7 @@ fn replace_unique_int_list() { r#" result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 when result is - Ok {list, value} -> list + Ok {list} -> list Err _ -> [] "# ), @@ -1727,7 +1727,7 @@ fn replace_unique_int_list_get_old_value() { r#" result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 when result is - Ok {list, value} -> value + Ok {value} -> value Err _ -> -1 "# ), From badad1c1ad49d5f03f0bfc57231e4d3eb05854d9 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:50:28 -0800 Subject: [PATCH 08/17] get basic tests passing --- compiler/gen_llvm/src/llvm/build_list.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index c6d39c33d5..483a07755b 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -309,7 +309,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( // Assume the bounds have already been checked earlier // (e.g. by List.replace or List.set, which wrap List.#replaceUnsafe) let new_list = match update_mode { - UpdateMode::InPlace => call_bitcode_fn( + UpdateMode::InPlace => call_list_bitcode_fn( env, &[ pass_list_cc(env, list), @@ -320,7 +320,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( ], bitcode::LIST_REPLACE_IN_PLACE, ), - UpdateMode::Immutable => call_bitcode_fn( + UpdateMode::Immutable => call_list_bitcode_fn( env, &[ pass_list_cc(env, list), @@ -351,11 +351,11 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( let result = env .builder - .build_insert_value(result, old_element, 0, "insert_value") + .build_insert_value(result, new_list, 0, "insert_list") .unwrap(); env.builder - .build_insert_value(result, new_list, 1, "insert_list") + .build_insert_value(result, old_element, 1, "insert_value") .unwrap() .into_struct_value() .into() From 116b585cdccb95897aefd2988ed1ca14c84732f6 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 24 Feb 2022 22:59:47 -0800 Subject: [PATCH 09/17] add more tests --- compiler/gen_llvm/src/llvm/build_list.rs | 1 - compiler/test_gen/src/gen_list.rs | 51 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index 483a07755b..ede3e6430a 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -340,7 +340,6 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( let result = env .context .struct_type( - // TODO: does the order need to be decided by the size of the element type. &[ super::convert::zig_list_type(env).into(), element_type.into(), diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index 18ce3051b5..2e3670c7fe 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1736,6 +1736,57 @@ fn replace_unique_int_list_get_old_value() { ); } +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn replace_unique_get_large_value() { + assert_evals_to!( + indoc!( + r#" + list : List { a : U64, b: U64, c: U64, d: U64 } + list = [ { a: 1, b: 2, c: 3, d: 4 }, { a: 5, b: 6, c: 7, d: 8 }, { a: 9, b: 10, c: 11, d: 12 } ] + result = List.replace list 1 { a: 13, b: 14, c: 15, d: 16 } + when result is + Ok {value} -> value + Err _ -> { a: 0, b: 0, c: 0, d: 0 } + "# + ), + (5, 6, 7, 8), + (u64, u64, u64, u64) + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn replace_shared_int_list() { + assert_evals_to!( + indoc!( + r#" + wrapper = \shared -> + # This should not mutate the original + replaced = + when List.replace shared 1 7.7 is + Ok {list} -> list + Err _ -> [] + x = + when List.get replaced 1 is + Ok num -> num + Err _ -> 0 + + y = + when List.get shared 1 is + Ok num -> num + Err _ -> 0 + + { x, y } + + wrapper [ 2.1, 4.3 ] + "# + ), + (7.7, 4.3), + (f64, f64) + ); +} + #[test] #[cfg(any(feature = "gen-llvm"))] fn get_set_unique_int_list() { From d9e9c28889e65ba8f960909738e75195998f67b0 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Sun, 27 Feb 2022 00:45:51 -0800 Subject: [PATCH 10/17] add error test case --- compiler/can/src/builtins.rs | 2 +- compiler/test_gen/src/gen_list.rs | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index 550c7d0d0f..dbc5063359 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -2326,7 +2326,7 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { // Otherwise, return the list unmodified. let body = If { cond_var: bool_var, - branch_var: var_store.fresh(), + branch_var: ret_result_var, branches: vec![( // if-condition no_region( diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index ba4cce5362..ac34090e7d 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1780,6 +1780,23 @@ fn replace_unique_int_list() { ); } +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn replace_unique_int_list_out_of_bounds() { + assert_evals_to!( + indoc!( + r#" + result = List.replace [ 12, 9, 7, 1, 5 ] 5 33 + when result is + Ok {value} -> value + Err OutOfBounds -> -1 + "# + ), + -1, + i64 + ); +} + #[test] #[cfg(any(feature = "gen-llvm"))] fn replace_unique_int_list_get_old_value() { From 4d42d81c63a786d16a66e17d55ad58ddfc154734 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Sun, 27 Feb 2022 01:21:02 -0800 Subject: [PATCH 11/17] add broken attempt to get list.set to use list.replace under the hood --- compiler/can/src/builtins.rs | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index dbc5063359..acec8fc84c 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -2408,6 +2408,7 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { let bool_var = var_store.fresh(); let len_var = var_store.fresh(); let elem_var = var_store.fresh(); + let replace_record_var = var_store.fresh(); let list_arg_var = var_store.fresh(); // Uniqueness type Attr differs between let list_ret_var = var_store.fresh(); // the arg list and the returned list @@ -2437,18 +2438,24 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { }, ), // then-branch - no_region( - // List.setUnsafe list index elem - RunLowLevel { - op: LowLevel::ListSet, - args: vec![ - (list_arg_var, Var(arg_list)), - (len_var, Var(arg_index)), - (elem_var, Var(arg_elem)), - ], - ret_var: list_ret_var, - }, - ), + no_region(Access { + record_var: replace_record_var, + ext_var: var_store.fresh(), + field_var: list_ret_var, + loc_expr: Box::new(no_region( + // List.replaceUnsafe list index elem + RunLowLevel { + op: LowLevel::ListReplaceUnsafe, + args: vec![ + (list_arg_var, Var(arg_list)), + (len_var, Var(arg_index)), + (elem_var, Var(arg_elem)), + ], + ret_var: replace_record_var, + }, + )), + field: "list".into(), + }), )], final_else: Box::new( // else-branch From 78fe73411375332fce48a0169645b5e7fd16ffe5 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 27 Feb 2022 22:47:08 +0100 Subject: [PATCH 12/17] change the return type of List.replace --- compiler/builtins/src/std.rs | 19 +++++------- compiler/can/src/builtins.rs | 57 ++++++++++++++++-------------------- 2 files changed, 34 insertions(+), 42 deletions(-) diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index 0ec5e0e28a..d9044f4f6e 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -1056,20 +1056,17 @@ pub fn types() -> MutMap { Box::new(result_type(flex(TVAR1), list_was_empty.clone())), ); - // replace : List elem, Nat, elem -> Result { list: List elem, value: elem } [ OutOfBounds ]* + // replace : List elem, Nat, elem -> { list: List elem, value: elem } add_top_level_function_type!( Symbol::LIST_REPLACE, vec![list_type(flex(TVAR1)), nat_type(), flex(TVAR1)], - Box::new(result_type( - SolvedType::Record { - fields: vec![ - ("list".into(), RecordField::Required(list_type(flex(TVAR1)))), - ("value".into(), RecordField::Required(flex(TVAR1))), - ], - ext: Box::new(SolvedType::EmptyRecord), - }, - index_out_of_bounds - )), + Box::new(SolvedType::Record { + fields: vec![ + ("list".into(), RecordField::Required(list_type(flex(TVAR1)))), + ("value".into(), RecordField::Required(flex(TVAR1))), + ], + ext: Box::new(SolvedType::EmptyRecord), + }), ); // set : List elem, Nat, elem -> List elem diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index acec8fc84c..3e26eb573e 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -2304,13 +2304,7 @@ fn list_get(symbol: Symbol, var_store: &mut VarStore) -> Def { ) } -/// List.replace : List elem, Nat, elem -> Result { list: List elem, value: elem } [ OutOfBounds ]* -/// -/// List.replace : -/// Attr (w | u | v) (List (Attr u a)), -/// Attr * Int, -/// Attr (u | v) a -/// -> Attr * (Result { list: List (Attr u a), value: Attr u a } (Attr * [ OutOfBounds ]*)) +/// List.replace : List elem, Nat, elem -> { list: List elem, value: elem } fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { let arg_list = Symbol::ARG_1; let arg_index = Symbol::ARG_2; @@ -2322,6 +2316,18 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { let ret_record_var = var_store.fresh(); let ret_result_var = var_store.fresh(); + let list_field = Field { + var: list_arg_var, + region: Region::zero(), + loc_expr: Box::new(Loc::at_zero(Expr::Var(arg_list))), + }; + + let value_field = Field { + var: elem_var, + region: Region::zero(), + loc_expr: Box::new(Loc::at_zero(Expr::Var(arg_elem))), + }; + // Perform a bounds check. If it passes, run LowLevel::ListReplaceUnsafe. // Otherwise, return the list unmodified. let body = If { @@ -2349,35 +2355,24 @@ fn list_replace(symbol: Symbol, var_store: &mut VarStore) -> Def { ), // then-branch no_region( - // Ok - tag( - "Ok", - vec![ - // List.replaceUnsafe list index elem - RunLowLevel { - op: LowLevel::ListReplaceUnsafe, - args: vec![ - (list_arg_var, Var(arg_list)), - (len_var, Var(arg_index)), - (elem_var, Var(arg_elem)), - ], - ret_var: ret_record_var, - }, + // List.replaceUnsafe list index elem + RunLowLevel { + op: LowLevel::ListReplaceUnsafe, + args: vec![ + (list_arg_var, Var(arg_list)), + (len_var, Var(arg_index)), + (elem_var, Var(arg_elem)), ], - var_store, - ), + ret_var: ret_record_var, + }, ), )], final_else: Box::new( // else-branch - no_region( - // Err - tag( - "Err", - vec![tag("OutOfBounds", Vec::new(), var_store)], - var_store, - ), - ), + no_region(record( + vec![("list".into(), list_field), ("value".into(), value_field)], + var_store, + )), ), }; From 81e45b1e2fda559590132f3609a401c42f4ff905 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 27 Feb 2022 22:47:26 +0100 Subject: [PATCH 13/17] make List.set use List.replace under the hood --- compiler/can/src/builtins.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index 3e26eb573e..95f198b25d 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -57,6 +57,7 @@ pub fn builtin_dependencies(symbol: Symbol) -> &'static [Symbol] { Symbol::LIST_PRODUCT => &[Symbol::LIST_WALK, Symbol::NUM_MUL], Symbol::LIST_SUM => &[Symbol::LIST_WALK, Symbol::NUM_ADD], Symbol::LIST_JOIN_MAP => &[Symbol::LIST_WALK, Symbol::LIST_CONCAT], + Symbol::LIST_SET => &[Symbol::LIST_REPLACE], _ => &[], } } @@ -2407,6 +2408,23 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { let list_arg_var = var_store.fresh(); // Uniqueness type Attr differs between let list_ret_var = var_store.fresh(); // the arg list and the returned list + let replace_function = ( + var_store.fresh(), + Loc::at_zero(Expr::Var(Symbol::LIST_REPLACE)), + var_store.fresh(), + replace_record_var, + ); + + let replace_call = Expr::Call( + Box::new(replace_function), + vec![ + (list_arg_var, Loc::at_zero(Var(arg_list))), + (len_var, Loc::at_zero(Var(arg_index))), + (elem_var, Loc::at_zero(Var(arg_elem))), + ], + CalledVia::Space, + ); + // Perform a bounds check. If it passes, run LowLevel::ListSet. // Otherwise, return the list unmodified. let body = If { @@ -2439,15 +2457,7 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { field_var: list_ret_var, loc_expr: Box::new(no_region( // List.replaceUnsafe list index elem - RunLowLevel { - op: LowLevel::ListReplaceUnsafe, - args: vec![ - (list_arg_var, Var(arg_list)), - (len_var, Var(arg_index)), - (elem_var, Var(arg_elem)), - ], - ret_var: replace_record_var, - }, + replace_call, )), field: "list".into(), }), From 2e70bb8458f530979553ca6d44cfd481a033cf59 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Sun, 27 Feb 2022 15:07:09 -0800 Subject: [PATCH 14/17] remove list set low level --- compiler/builtins/bitcode/src/list.zig | 91 ------------------------ compiler/builtins/bitcode/src/main.zig | 2 - compiler/builtins/src/bitcode.rs | 2 - compiler/gen_llvm/src/llvm/build.rs | 17 +---- compiler/gen_llvm/src/llvm/build_list.rs | 48 ------------- compiler/gen_wasm/src/low_level.rs | 2 +- compiler/module/src/low_level.rs | 2 - compiler/mono/src/alias_analysis.rs | 8 +-- compiler/mono/src/borrow.rs | 1 - 9 files changed, 5 insertions(+), 168 deletions(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 299f4f1591..f9ef75410f 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -1308,97 +1308,6 @@ inline fn listReplaceInPlaceHelp( return list; } -pub fn listSetInPlace( - bytes: ?[*]u8, - index: usize, - element: Opaque, - element_width: usize, - dec: Dec, -) callconv(.C) ?[*]u8 { - // INVARIANT: bounds checking happens on the roc side - // - // at the time of writing, the function is implemented roughly as - // `if inBounds then LowLevelListGet input index item else input` - // so we don't do a bounds check here. Hence, the list is also non-empty, - // because inserting into an empty list is always out of bounds - - return listSetInPlaceHelp(bytes, index, element, element_width, dec); -} - -pub fn listSet( - bytes: ?[*]u8, - length: usize, - alignment: u32, - index: usize, - element: Opaque, - element_width: usize, - dec: Dec, -) callconv(.C) ?[*]u8 { - // INVARIANT: bounds checking happens on the roc side - // - // at the time of writing, the function is implemented roughly as - // `if inBounds then LowLevelListGet input index item else input` - // so we don't do a bounds check here. Hence, the list is also non-empty, - // because inserting into an empty list is always out of bounds - const ptr: [*]usize = @ptrCast([*]usize, @alignCast(@alignOf(usize), bytes)); - - if ((ptr - 1)[0] == utils.REFCOUNT_ONE) { - return listSetInPlaceHelp(bytes, index, element, element_width, dec); - } else { - return listSetImmutable(bytes, length, alignment, index, element, element_width, dec); - } -} - -inline fn listSetInPlaceHelp( - bytes: ?[*]u8, - index: usize, - element: Opaque, - element_width: usize, - dec: Dec, -) ?[*]u8 { - // the element we will replace - var element_at_index = (bytes orelse undefined) + (index * element_width); - - // decrement its refcount - dec(element_at_index); - - // copy in the new element - @memcpy(element_at_index, element orelse undefined, element_width); - - return bytes; -} - -inline fn listSetImmutable( - old_bytes: ?[*]u8, - length: usize, - alignment: u32, - index: usize, - element: Opaque, - element_width: usize, - dec: Dec, -) ?[*]u8 { - const data_bytes = length * element_width; - - var new_bytes = utils.allocateWithRefcount(data_bytes, alignment); - - @memcpy(new_bytes, old_bytes orelse undefined, data_bytes); - - // the element we will replace - var element_at_index = new_bytes + (index * element_width); - - // decrement its refcount - dec(element_at_index); - - // copy in the new element - @memcpy(element_at_index, element orelse undefined, element_width); - - // consume RC token of original - utils.decref(old_bytes, data_bytes, alignment); - - //return list; - return new_bytes; -} - pub fn listFindUnsafe( list: RocList, caller: Caller1, diff --git a/compiler/builtins/bitcode/src/main.zig b/compiler/builtins/bitcode/src/main.zig index 8a23f09985..5388804416 100644 --- a/compiler/builtins/bitcode/src/main.zig +++ b/compiler/builtins/bitcode/src/main.zig @@ -51,8 +51,6 @@ comptime { exportListFn(list.listDropAt, "drop_at"); exportListFn(list.listReplace, "replace"); exportListFn(list.listReplaceInPlace, "replace_in_place"); - exportListFn(list.listSet, "set"); - exportListFn(list.listSetInPlace, "set_in_place"); exportListFn(list.listSwap, "swap"); exportListFn(list.listAny, "any"); exportListFn(list.listAll, "all"); diff --git a/compiler/builtins/src/bitcode.rs b/compiler/builtins/src/bitcode.rs index c4c0b12e01..5be3ecb830 100644 --- a/compiler/builtins/src/bitcode.rs +++ b/compiler/builtins/src/bitcode.rs @@ -356,8 +356,6 @@ pub const LIST_SORT_WITH: &str = "roc_builtins.list.sort_with"; pub const LIST_CONCAT: &str = "roc_builtins.list.concat"; pub const LIST_REPLACE: &str = "roc_builtins.list.replace"; pub const LIST_REPLACE_IN_PLACE: &str = "roc_builtins.list.replace_in_place"; -pub const LIST_SET: &str = "roc_builtins.list.set"; -pub const LIST_SET_IN_PLACE: &str = "roc_builtins.list.set_in_place"; pub const LIST_ANY: &str = "roc_builtins.list.any"; pub const LIST_ALL: &str = "roc_builtins.list.all"; pub const LIST_FIND_UNSAFE: &str = "roc_builtins.list.find_unsafe"; diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index d3be9df353..a80b1becb4 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -14,7 +14,7 @@ use crate::llvm::build_list::{ list_contains, list_drop_at, list_find_unsafe, list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len, list_map, list_map2, list_map3, list_map4, list_map_with_index, list_prepend, list_range, list_repeat, list_replace_unsafe, list_reverse, - list_set, list_single, list_sort_with, list_sublist, list_swap, + list_single, list_sort_with, list_sublist, list_swap, }; use crate::llvm::build_str::{ str_concat, str_count_graphemes, str_ends_with, str_from_float, str_from_int, str_from_utf8, @@ -5681,21 +5681,6 @@ fn run_low_level<'a, 'ctx, 'env>( update_mode, ) } - ListSet => { - let list = load_symbol(scope, &args[0]); - let index = load_symbol(scope, &args[1]); - let (element, element_layout) = load_symbol_and_layout(scope, &args[2]); - - list_set( - env, - layout_ids, - list, - index.into_int_value(), - element, - element_layout, - update_mode, - ) - } NumToStr => { // Num.toStr : Num a -> Str debug_assert_eq!(args.len(), 1); diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index ede3e6430a..c818ed4d23 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -360,54 +360,6 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( .into() } -/// List.set : List elem, Nat, elem -> List elem -pub fn list_set<'a, 'ctx, 'env>( - env: &Env<'a, 'ctx, 'env>, - layout_ids: &mut LayoutIds<'a>, - list: BasicValueEnum<'ctx>, - index: IntValue<'ctx>, - element: BasicValueEnum<'ctx>, - element_layout: &Layout<'a>, - update_mode: UpdateMode, -) -> BasicValueEnum<'ctx> { - let dec_element_fn = build_dec_wrapper(env, layout_ids, element_layout); - - let (length, bytes) = load_list( - env.builder, - list.into_struct_value(), - env.context.i8_type().ptr_type(AddressSpace::Generic), - ); - - let new_bytes = match update_mode { - UpdateMode::InPlace => call_bitcode_fn( - env, - &[ - bytes.into(), - index.into(), - pass_element_as_opaque(env, element, *element_layout), - layout_width(env, element_layout), - dec_element_fn.as_global_value().as_pointer_value().into(), - ], - bitcode::LIST_SET_IN_PLACE, - ), - UpdateMode::Immutable => call_bitcode_fn( - env, - &[ - bytes.into(), - length.into(), - env.alignment_intvalue(element_layout), - index.into(), - pass_element_as_opaque(env, element, *element_layout), - layout_width(env, element_layout), - dec_element_fn.as_global_value().as_pointer_value().into(), - ], - bitcode::LIST_SET, - ), - }; - - store_list(env, new_bytes.into_pointer_value(), length) -} - fn bounds_check_comparison<'ctx>( builder: &Builder<'ctx>, elem_index: IntValue<'ctx>, diff --git a/compiler/gen_wasm/src/low_level.rs b/compiler/gen_wasm/src/low_level.rs index b68d8e968a..f22ebd0bbb 100644 --- a/compiler/gen_wasm/src/low_level.rs +++ b/compiler/gen_wasm/src/low_level.rs @@ -260,7 +260,7 @@ impl<'a> LowLevelCall<'a> { _ => internal_error!("invalid storage for List"), }, - ListGetUnsafe | ListReplaceUnsafe | ListSet | ListSingle | ListRepeat | ListReverse + ListGetUnsafe | ListReplaceUnsafe | ListSingle | ListRepeat | ListReverse | ListConcat | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap | ListMap2 | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf | ListWalk | ListWalkUntil | ListWalkBackwards | ListKeepOks | ListKeepErrs diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index ad10a1cbba..f7b3987be1 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -25,7 +25,6 @@ pub enum LowLevel { StrToNum, ListLen, ListGetUnsafe, - ListSet, ListSingle, ListRepeat, ListReplaceUnsafe, @@ -229,7 +228,6 @@ impl LowLevelWrapperType { Symbol::STR_TO_I8 => WrapperIsRequired, Symbol::LIST_LEN => CanBeReplacedBy(ListLen), Symbol::LIST_GET => WrapperIsRequired, - Symbol::LIST_SET => WrapperIsRequired, Symbol::LIST_REPLACE => WrapperIsRequired, Symbol::LIST_SINGLE => CanBeReplacedBy(ListSingle), Symbol::LIST_REPEAT => CanBeReplacedBy(ListRepeat), diff --git a/compiler/mono/src/alias_analysis.rs b/compiler/mono/src/alias_analysis.rs index f25e3e3b08..f104a2f20a 100644 --- a/compiler/mono/src/alias_analysis.rs +++ b/compiler/mono/src/alias_analysis.rs @@ -1258,19 +1258,17 @@ fn lowlevel_spec( builder.add_bag_get(block, bag) } - ListSet => { + ListReplaceUnsafe => { let list = env.symbols[&arguments[0]]; let to_insert = env.symbols[&arguments[2]]; let bag = builder.add_get_tuple_field(block, list, LIST_BAG_INDEX)?; let cell = builder.add_get_tuple_field(block, list, LIST_CELL_INDEX)?; - // decrement the overwritten element - let overwritten = builder.add_bag_get(block, bag)?; - let _unit = builder.add_recursive_touch(block, overwritten)?; - let _unit = builder.add_update(block, update_mode_var, cell)?; + // replace loads the old element and then inserts the new one. + builder.add_bag_get(block, bag)?; builder.add_bag_insert(block, bag, to_insert)?; with_new_heap_cell(builder, block, bag) diff --git a/compiler/mono/src/borrow.rs b/compiler/mono/src/borrow.rs index d46ccbc8ee..2d081fb7b4 100644 --- a/compiler/mono/src/borrow.rs +++ b/compiler/mono/src/borrow.rs @@ -934,7 +934,6 @@ pub fn lowlevel_borrow_signature(arena: &Bump, op: LowLevel) -> &[bool] { // - other refcounted arguments are Borrowed match op { ListLen | StrIsEmpty | StrCountGraphemes => arena.alloc_slice_copy(&[borrowed]), - ListSet => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), ListReplaceUnsafe => arena.alloc_slice_copy(&[owned, irrelevant, irrelevant]), ListGetUnsafe => arena.alloc_slice_copy(&[borrowed, irrelevant]), ListConcat => arena.alloc_slice_copy(&[owned, owned]), From 457ba524aacda0d3ca08bde34623d0f057dcf822 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Sun, 27 Feb 2022 15:20:54 -0800 Subject: [PATCH 15/17] fix tests and alias analysis --- compiler/mono/src/alias_analysis.rs | 9 +++++---- compiler/test_gen/src/gen_list.rs | 31 ++++++++++------------------- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/compiler/mono/src/alias_analysis.rs b/compiler/mono/src/alias_analysis.rs index f104a2f20a..db061cf395 100644 --- a/compiler/mono/src/alias_analysis.rs +++ b/compiler/mono/src/alias_analysis.rs @@ -1265,13 +1265,14 @@ fn lowlevel_spec( let bag = builder.add_get_tuple_field(block, list, LIST_BAG_INDEX)?; let cell = builder.add_get_tuple_field(block, list, LIST_CELL_INDEX)?; - let _unit = builder.add_update(block, update_mode_var, cell)?; + let _unit1 = builder.add_touch(block, cell)?; + let _unit2 = builder.add_update(block, update_mode_var, cell)?; - // replace loads the old element and then inserts the new one. - builder.add_bag_get(block, bag)?; builder.add_bag_insert(block, bag, to_insert)?; - with_new_heap_cell(builder, block, bag) + let old_value = builder.add_bag_get(block, bag)?; + let new_list = with_new_heap_cell(builder, block, bag)?; + builder.add_make_tuple(block, &[new_list, old_value]) } ListSwap => { let list = env.symbols[&arguments[0]]; diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index ac34090e7d..8eb784c2b8 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -1769,10 +1769,8 @@ fn replace_unique_int_list() { assert_evals_to!( indoc!( r#" - result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 - when result is - Ok {list} -> list - Err _ -> [] + record = List.replace [ 12, 9, 7, 1, 5 ] 2 33 + record.list "# ), RocList::from_slice(&[12, 9, 33, 1, 5]), @@ -1786,13 +1784,11 @@ fn replace_unique_int_list_out_of_bounds() { assert_evals_to!( indoc!( r#" - result = List.replace [ 12, 9, 7, 1, 5 ] 5 33 - when result is - Ok {value} -> value - Err OutOfBounds -> -1 + record = List.replace [ 12, 9, 7, 1, 5 ] 5 33 + record.value "# ), - -1, + 33, i64 ); } @@ -1803,10 +1799,8 @@ fn replace_unique_int_list_get_old_value() { assert_evals_to!( indoc!( r#" - result = List.replace [ 12, 9, 7, 1, 5 ] 2 33 - when result is - Ok {value} -> value - Err _ -> -1 + record = List.replace [ 12, 9, 7, 1, 5 ] 2 33 + record.value "# ), 7, @@ -1822,10 +1816,8 @@ fn replace_unique_get_large_value() { r#" list : List { a : U64, b: U64, c: U64, d: U64 } list = [ { a: 1, b: 2, c: 3, d: 4 }, { a: 5, b: 6, c: 7, d: 8 }, { a: 9, b: 10, c: 11, d: 12 } ] - result = List.replace list 1 { a: 13, b: 14, c: 15, d: 16 } - when result is - Ok {value} -> value - Err _ -> { a: 0, b: 0, c: 0, d: 0 } + record = List.replace list 1 { a: 13, b: 14, c: 15, d: 16 } + record.value "# ), (5, 6, 7, 8), @@ -1841,10 +1833,7 @@ fn replace_shared_int_list() { r#" wrapper = \shared -> # This should not mutate the original - replaced = - when List.replace shared 1 7.7 is - Ok {list} -> list - Err _ -> [] + replaced = (List.replace shared 1 7.7).list x = when List.get replaced 1 is Ok num -> num From 2ca1ebdd2d380a1676346d75c15a4a104a4c3d2c Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Sun, 27 Feb 2022 15:50:19 -0800 Subject: [PATCH 16/17] appease the paperclip --- compiler/builtins/src/std.rs | 2 +- compiler/gen_llvm/src/llvm/build_list.rs | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index d9044f4f6e..8c6e96bbd4 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -1034,7 +1034,7 @@ pub fn types() -> MutMap { add_top_level_function_type!( Symbol::LIST_GET, vec![list_type(flex(TVAR1)), nat_type()], - Box::new(result_type(flex(TVAR1), index_out_of_bounds.clone())), + Box::new(result_type(flex(TVAR1), index_out_of_bounds)), ); // first : List elem -> Result elem [ ListWasEmpty ]* diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index c818ed4d23..dca6e49466 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -301,7 +301,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( element_layout: &Layout<'a>, update_mode: UpdateMode, ) -> BasicValueEnum<'ctx> { - let element_type = basic_type_from_layout(env, &element_layout); + let element_type = basic_type_from_layout(env, element_layout); let element_ptr = env .builder .build_alloca(element_type, "output_element_as_opaque"); @@ -340,10 +340,7 @@ pub fn list_replace_unsafe<'a, 'ctx, 'env>( let result = env .context .struct_type( - &[ - super::convert::zig_list_type(env).into(), - element_type.into(), - ], + &[super::convert::zig_list_type(env).into(), element_type], false, ) .const_zero(); From 6fae7039c2be40e04b0b35808d315c3ae4d32ad8 Mon Sep 17 00:00:00 2001 From: Folkert Date: Tue, 1 Mar 2022 21:56:10 +0100 Subject: [PATCH 17/17] remove unused function --- compiler/types/src/builtin_aliases.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/compiler/types/src/builtin_aliases.rs b/compiler/types/src/builtin_aliases.rs index 07efa07c9e..f8df53a256 100644 --- a/compiler/types/src/builtin_aliases.rs +++ b/compiler/types/src/builtin_aliases.rs @@ -914,15 +914,6 @@ pub fn ordering_type() -> SolvedType { ) } -#[inline(always)] -pub fn pair_type(t1: SolvedType, t2: SolvedType) -> SolvedType { - // [ Pair t1 t2 ] - SolvedType::TagUnion( - vec![(TagName::Global("Pair".into()), vec![t1, t2])], - Box::new(SolvedType::EmptyTagUnion), - ) -} - #[inline(always)] pub fn result_type(a: SolvedType, e: SolvedType) -> SolvedType { SolvedType::Alias(