Skip to content

Commit

Permalink
Merge pull request #6907 from roc-lang/refcount-followup
Browse files Browse the repository at this point in the history 
Refcount followup
  • Loading branch information
@bhansconnect
bhansconnect authored Jul 16, 2024
2 parents 5cf4a4a + 2779145 commit 5b87d82
Show file tree
Hide file tree
Showing 7 changed files with 194 additions and 130 deletions.
2 changes: 1 addition & 1 deletion crates/compiler/mono/src/code_gen_help/refcount.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub fn refcount_stmt<'a>(
ctx,
layout_interner,
layout,
modify,
&ModifyRc::Dec(*structure),
following,
)
}
Expand Down
246 changes: 132 additions & 114 deletions crates/compiler/mono/src/drop_specialization.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use roc_module::low_level::LowLevel;
use roc_module::symbol::{IdentIds, ModuleId, Symbol};

use crate::ir::{
BranchInfo, Call, CallType, ErasedField, Expr, JoinPointId, ListLiteralElement, Literal,
ModifyRc, Proc, ProcLayout, Stmt, UpdateModeId,
BranchInfo, Call, CallType, ErasedField, Expr, JoinPointId, ListLiteralElement, ModifyRc, Proc,
ProcLayout, Stmt, UpdateModeId,
};
use crate::layout::{
Builtin, InLayout, Layout, LayoutInterner, LayoutRepr, STLayoutInterner, UnionLayout,
Expand Down Expand Up @@ -1075,13 +1075,11 @@ fn specialize_list<'a, 'i>(
layout_interner: &'i mut STLayoutInterner<'a>,
ident_ids: &'i mut IdentIds,
environment: &mut DropSpecializationEnvironment<'a>,
incremented_children: &mut CountingMap<Child>,
_incremented_children: &mut CountingMap<Child>,
symbol: &Symbol,
item_layout: InLayout<'a>,
_item_layout: InLayout<'a>,
continuation: &'a Stmt<'a>,
) -> &'a Stmt<'a> {
let current_length = environment.list_length.get(symbol).copied();

macro_rules! keep_original_decrement {
() => {{
let new_continuation =
Expand All @@ -1090,114 +1088,134 @@ fn specialize_list<'a, 'i>(
}};
}

match (
layout_interner.contains_refcounted(item_layout),
current_length,
) {
// Only specialize lists if the amount of children is known.
// Otherwise we might have to insert an unbouned number of decrements.
(true, Some(length)) => {
match environment.list_children.get(symbol) {
Some(children) => {
// TODO perhaps this allocation can be avoided.
let children_clone = children.clone();

// Map tracking which index of the struct is contained in which symbol.
// And whether the child no longer has to be decremented.
let mut index_symbols = MutMap::default();

for index in 0..length {
for (child, i) in children_clone
.iter()
.rev()
.filter(|(_child, i)| *i == index)
{
debug_assert!(length > *i);

let removed = incremented_children.pop(child);
index_symbols.insert(index, (*child, removed));

if removed {
break;
}
}
}

let new_continuation = specialize_drops_stmt(
arena,
layout_interner,
ident_ids,
environment,
continuation,
);

let mut newer_continuation = arena.alloc(Stmt::Refcounting(
ModifyRc::DecRef(*symbol),
new_continuation,
));

// Reversed to ensure that the generated code decrements the items in the correct order.
for i in (0..length).rev() {
match index_symbols.get(&i) {
// If the symbol is known, we can decrement it (if incremented before).
Some((s, popped)) => {
if !*popped {
// Decrement the children that were not incremented before. And thus don't cancel out.
newer_continuation = arena.alloc(Stmt::Refcounting(
ModifyRc::Dec(*s),
newer_continuation,
));
}

// Do nothing for the children that were incremented before, as the decrement will cancel out.
}
// If the symbol is unknown, we have to get the value from the list.
// Should only happen when list elements are discarded.
None => {
let field_symbol =
environment.create_symbol(ident_ids, &format!("field_val_{i}"));

let index_symbol =
environment.create_symbol(ident_ids, &format!("index_val_{i}"));

let dec = arena.alloc(Stmt::Refcounting(
ModifyRc::Dec(field_symbol),
newer_continuation,
));

let index = arena.alloc(Stmt::Let(
field_symbol,
Expr::Call(Call {
call_type: CallType::LowLevel {
op: LowLevel::ListGetUnsafe,
update_mode: UpdateModeId::BACKEND_DUMMY,
},
arguments: arena.alloc([*symbol, index_symbol]),
}),
item_layout,
dec,
));

newer_continuation = arena.alloc(Stmt::Let(
index_symbol,
Expr::Literal(Literal::Int(i128::to_ne_bytes(i as i128))),
Layout::isize(layout_interner.target()),
index,
));
}
};
}

newer_continuation
}
_ => keep_original_decrement!(),
}
}
_ => {
// List length is unknown or the children are not reference counted, so we can't specialize.
keep_original_decrement!()
}
}
// TODO: Maybe re-enable drop specialization for lists.
// It won't be as useful now, but it can still apply to lists that we know aren't seamless slices.
// It also could technically apply to seamless slices if we know everything about their underlying allocation.
// To fix this would require adding the restrictions on application above and properly implementing DecRef for the new list.
// DecRef would be easy to implement, but currently the new list has Dec/DecRef both as the same.
// Extra context starts here: https://roc.zulipchat.com/#narrow/stream/316715-contributing/topic/Implement.20RocRefcounted.20for.20RocResult/near/451651633
// With most important message here: https://roc.zulipchat.com/#narrow/stream/316715-contributing/topic/Implement.20RocRefcounted.20for.20RocResult/near/451692266
//
// If we don't re-enable this, we should fully remove the list wiring and trackind at some point.
//
// Fundamentally, an allocation for a list holds onto exactly one reference to all elements in the allocation.
// So even if there are 100 references to the list, the list will hold only 1 reference to each element.
// On top of that, the list now holds onto a reference to dead elements.
// A single slice to 10 elements of a 100 element list will hold 1 reference to each to the 100 elements.
// The whole allocation will get freed in a unit.
// This makes it much closer to the lifetime of the underlying vector for a reference slice in rust.
// If this specialization does not make sense anymore delete it as a whole.
keep_original_decrement!()

// let current_length = environment.list_length.get(symbol).copied();
// match (
// layout_interner.contains_refcounted(item_layout),
// current_length,
// ) {
// // Only specialize lists if the amount of children is known.
// // Otherwise we might have to insert an unbouned number of decrements.
// (true, Some(length)) => {
// match environment.list_children.get(symbol) {
// Some(children) => {
// // TODO perhaps this allocation can be avoided.
// let children_clone = children.clone();

// // Map tracking which index of the struct is contained in which symbol.
// // And whether the child no longer has to be decremented.
// let mut index_symbols = MutMap::default();

// for index in 0..length {
// for (child, i) in children_clone
// .iter()
// .rev()
// .filter(|(_child, i)| *i == index)
// {
// debug_assert!(length > *i);

// let removed = incremented_children.pop(child);
// index_symbols.insert(index, (*child, removed));

// if removed {
// break;
// }
// }
// }

// let new_continuation = specialize_drops_stmt(
// arena,
// layout_interner,
// ident_ids,
// environment,
// continuation,
// );

// let mut newer_continuation = arena.alloc(Stmt::Refcounting(
// ModifyRc::DecRef(*symbol),
// new_continuation,
// ));

// // Reversed to ensure that the generated code decrements the items in the correct order.
// for i in (0..length).rev() {
// match index_symbols.get(&i) {
// // If the symbol is known, we can decrement it (if incremented before).
// Some((s, popped)) => {
// if !*popped {
// // Decrement the children that were not incremented before. And thus don't cancel out.
// newer_continuation = arena.alloc(Stmt::Refcounting(
// ModifyRc::Dec(*s),
// newer_continuation,
// ));
// }

// // Do nothing for the children that were incremented before, as the decrement will cancel out.
// }
// // If the symbol is unknown, we have to get the value from the list.
// // Should only happen when list elements are discarded.
// None => {
// let field_symbol =
// environment.create_symbol(ident_ids, &format!("field_val_{i}"));

// let index_symbol =
// environment.create_symbol(ident_ids, &format!("index_val_{i}"));

// let dec = arena.alloc(Stmt::Refcounting(
// ModifyRc::Dec(field_symbol),
// newer_continuation,
// ));

// let index = arena.alloc(Stmt::Let(
// field_symbol,
// Expr::Call(Call {
// call_type: CallType::LowLevel {
// op: LowLevel::ListGetUnsafe,
// update_mode: UpdateModeId::BACKEND_DUMMY,
// },
// arguments: arena.alloc([*symbol, index_symbol]),
// }),
// item_layout,
// dec,
// ));

// newer_continuation = arena.alloc(Stmt::Let(
// index_symbol,
// Expr::Literal(Literal::Int(i128::to_ne_bytes(i as i128))),
// Layout::isize(layout_interner.target()),
// index,
// ));
// }
// };
// }

// newer_continuation
// }
// _ => keep_original_decrement!(),
// }
// }
// _ => {
// // List length is unknown or the children are not reference counted, so we can't specialize.
// keep_original_decrement!()
// }
// }
}

/**
Expand Down
24 changes: 22 additions & 2 deletions crates/compiler/test_gen/src/gen_list.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3912,7 +3912,6 @@ fn list_range_length_overflow() {
);
}

#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))]
mod pattern_match {
#[allow(unused_imports)]
use crate::helpers::with_larger_debug_stack;
Expand All @@ -3926,9 +3925,10 @@ mod pattern_match {
#[cfg(feature = "gen-dev")]
use crate::helpers::dev::assert_evals_to;

use super::RocList;
use super::{RocList, RocStr};

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn unary_exact_size_match() {
assert_evals_to!(
r"
Expand All @@ -3944,6 +3944,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn many_exact_size_match() {
assert_evals_to!(
r"
Expand All @@ -3962,6 +3963,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))]
fn ranged_matches_head() {
with_larger_debug_stack(|| {
assert_evals_to!(
Expand Down Expand Up @@ -3994,6 +3996,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))]
fn ranged_matches_tail() {
with_larger_debug_stack(|| {
assert_evals_to!(
Expand Down Expand Up @@ -4026,6 +4029,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn bind_variables() {
assert_evals_to!(
r"
Expand Down Expand Up @@ -4054,6 +4058,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))]
fn order_list_size_tests_issue_4732() {
assert_evals_to!(
r"
Expand Down Expand Up @@ -4101,6 +4106,7 @@ mod pattern_match {
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn rest_as() {
assert_evals_to!(
r"
Expand All @@ -4122,6 +4128,20 @@ mod pattern_match {
RocList<u8>
)
}

#[test]
#[cfg(any(feature = "gen-llvm", feature = "gen-wasm", feature = "gen-dev"))]
fn list_str() {
assert_evals_to!(
r#"
when ["d"] is
[alone] -> [alone]
other -> other
"#,
RocList::from_slice(&[RocStr::from("d")]),
RocList<RocStr>
)
}
}

#[test]
Expand Down
Loading

0 comments on commit 5b87d82

Please sign in to comment.