Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize literal list construction in LLVM backend #6832

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ayazhafiz
Copy link
Sponsor Member

Currently, list literals are always heap-allocated and their elements
are stored by emitting a GEP and store for each item in the literal.
This produces huge quantities of IR, causing compile times for e.g.
programs with large literals or ingested files to blow up.

Instead, if a list literal consists entirely of literal values, create a
global section for the literal and return a pointer to it.

@lukewilliamboswell
Copy link
Collaborator

Tested this using roc-htmx-playground, massive speed up in compile time, and significantly smaller LLVM IR file produced.

crates/compiler/gen_llvm/src/llvm/build.rs Outdated Show resolved Hide resolved
crates/compiler/gen_llvm/src/llvm/build.rs Show resolved Hide resolved
crates/compiler/gen_llvm/src/llvm/build.rs Outdated Show resolved Hide resolved
crates/compiler/gen_llvm/src/llvm/build.rs Outdated Show resolved Hide resolved
Copy link

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

ayazhafiz and others added 2 commits August 9, 2024 18:51
Currently, list literals are always heap-allocated and their elements
are stored by emitting a GEP and store for each item in the literal.
This produces huge quantities of IR, causing compile times for e.g.
programs with large literals or ingested files to blow up.

Instead, if a list literal consists entirely of literal values, create a
global section for the literal and return a pointer to it.
@bhansconnect bhansconnect force-pushed the ayaz/optimize-list-literal-alloc branch from cc8767d to 4628725 Compare August 10, 2024 02:07
@bhansconnect
Copy link
Contributor

Ok, added the changes mentioned above as required. I think this should pass CI. We shall see.

Comment on lines 3015 to 3041
} else {
// some of our elements are non-constant, so we must allocate space on the heap
let ptr = allocate_list(env, layout_interner, element_layout, list_length_intval);
let data_ptr = allocate_list(env, layout_interner, element_layout, list_length_intval);

// then, copy the relevant segment from the constant section into the heap
env.builder
.build_memcpy(
ptr,
alignment,
global,
alignment,
env.ptr_int().const_int(size as _, false),
)
.unwrap();

// then replace the `undef`s with the values that we evaluate at runtime
for (index, val) in runtime_evaluated_elements {
let index_val = ctx.i64_type().const_int(index as u64, false);
let elem_ptr = unsafe {
builder.new_build_in_bounds_gep(element_type, ptr, &[index_val], "index")
};

builder.new_build_store(elem_ptr, val);
}
let byte_size = env
.ptr_int()
.const_int(list_length as u64 * element_width as u64, false);
builder
.build_memcpy(data_ptr, alignment, const_data_ptr, alignment, byte_size)
.unwrap();

super::build_list::store_list(env, ptr, list_length_intval).into()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question: How much do we care about this partially constant case? The old code was using a global for partially constant lists. The new code only handles fully constant lists.

@bhansconnect
Copy link
Contributor

Oh cool, its actually passing

@lukewilliamboswell
Copy link
Collaborator

@bhansconnect do you know what the status of this is? can we merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants