Skip to content

Make Ctx be Send + Sync so Cranelift and Wasmtime can reuse it #263

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.

Sign up for GitHub

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

Merged
cfallin merged 1 commit into from
Jul 20, 2026
Merged

Make Ctx be Send + Sync so Cranelift and Wasmtime can reuse it #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 132 additions & 5 deletions src/ion/data_structures.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,71 @@ impl core::ops::IndexMut<VReg> for VRegs {
}
}

#[derive(Default)]
/// An unsafe wrapper around a `T` that is always `Send + Sync`, regardless
/// whether `T` is or not.
#[repr(transparent)]
pub(crate) struct UnsafeSendSync<T> {
inner: T,
}

impl<'a, T> IntoIterator for &'a UnsafeSendSync<T>
where
&'a T: IntoIterator,
{
type Item = <&'a T as IntoIterator>::Item;

type IntoIter = <&'a T as IntoIterator>::IntoIter;

fn into_iter(self) -> Self::IntoIter {
(&self.inner).into_iter()
}
}

impl<'a, T> IntoIterator for &'a mut UnsafeSendSync<T>
where
&'a mut T: IntoIterator,
{
type Item = <&'a mut T as IntoIterator>::Item;

type IntoIter = <&'a mut T as IntoIterator>::IntoIter;

fn into_iter(self) -> Self::IntoIter {
(&mut self.inner).into_iter()
}
}

// SAFETY: upheld by the invariants callers of `UnsafeSendSync::new` must uphold
// in turn.
unsafe impl<T> Send for UnsafeSendSync<T> {}
unsafe impl<T> Sync for UnsafeSendSync<T> {}

impl<T> Deref for UnsafeSendSync<T> {
type Target = T;

fn deref(&self) -> &Self::Target {
&self.inner
}
}

impl<T> DerefMut for UnsafeSendSync<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.inner
}
}

impl<T> UnsafeSendSync<T> {
/// Create a new `UnsafeSendSync<T>`.
///
/// # Safety
///
/// The result is `Send` and `Sync` regardless whether `T: Send + Sync`. You
/// must ensure that the result is only used in a manner that is compatible
/// with whether `T` is `Send + Sync`.
pub unsafe fn new(inner: T) -> Self {
Self { inner }
}
}

pub struct Ctx {
pub(crate) cfginfo: CFGInfo,
pub(crate) cfginfo_ctx: CFGInfoCtx,
Expand All @@ -453,10 +517,10 @@ pub struct Ctx {
pub(crate) blockparam_outs: Vec<BlockparamOut>,
pub(crate) blockparam_ins: Vec<BlockparamIn>,

pub(crate) ranges: LiveRanges,
pub(crate) bundles: LiveBundles,
pub(crate) ranges: UnsafeSendSync<LiveRanges>,
pub(crate) bundles: UnsafeSendSync<LiveBundles>,
pub(crate) spillsets: SpillSets,
pub(crate) vregs: VRegs,
pub(crate) vregs: UnsafeSendSync<VRegs>,
pub(crate) pregs: Vec<PRegData>,
pub(crate) allocation_queue: PrioQueue,

Expand Down Expand Up @@ -503,7 +567,70 @@ pub struct Ctx {
pub(crate) scratch_removed_lrs_vregs: FxHashSet<VRegIndex>,
pub(crate) scratch_workqueue_set: FxHashSet<Block>,

pub(crate) scratch_bump: Bump,
pub(crate) scratch_bump: UnsafeSendSync<Bump>,
}

const _ASSERT_SEND_SYNC: () = {
const fn assert_send_sync<T: Send + Sync>() {}
assert_send_sync::<Ctx>();
};

impl Default for Ctx {
fn default() -> Self {
Self {
// SAFETY: These collections use a `Bump` allocator. `Bump` is not
// `Send + Sync` because it is a newtype of `Rc<bumpalo::Bump>`,
// which leads to two problems:
//
// 1. `bumpalo::Bump` is `Send` but it is _not_ `Sync` (it internally uses a
// `Cell`), and
//
// 2. `Rc` is neither `Send` nor `Sync`.
//
// Nonetheless, we only ever touch a `Bump` (and every clone of it!)
// from a single thread at a time: all clones of a given `Bump` live
// inside one `Ctx`; a `Ctx` is created, mutated, and dropped by a
// single thread at a time; and the returned `Output` is

@cfallin cfallin Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I have quite mixed feelings about this: if I'm reading it right, it's asserting statements that regalloc2 cannot guarantee ("a Ctx is created, mutated, and dropped by a single thread at a time") and are properties ensured only by the user/embedder of the library, so would necessarily imply the main regalloc2 entry points that take Ctx become unsafe. I don't like that both on principle and because it seems like a somewhat not-unlikely footgun: if we ever break up compilation into smaller units of work and use a thread pool, for example, we suddenly have hidden invariants about pieces of the Cranelift+regalloc2 context and thread-locality that are no longer enforced by the type system. I pretty strongly don't want the Cranelift-to-regalloc2 API to be fundamentally unsafe.

Would it be possible to create a Bump variant that internally uses an Arc instead?

(Or am I misunderstanding how the invariants work here?)

@cfallin cfallin Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, talked to Nick about this offline and realized that Ctx itself is still safe at the regalloc2 API boundary; the unsafety is purely in the knowledge that Rc is morally "Send if everything is ported over to another thread in one unit" (i.e., we don't have a trait to talk about move-the-whole-graph vs. move-this-one-object, so Rc and similar types with thread-unsafe bookkeeping ban moving any piece at all). In effect Ctx encapsulates the inner Rc and everything that could share with it (other cloned copies of the Bump inside containers or whatever) so moving Ctx's ownership across threads is safe.

So, no objection and this seems reasonable!

// `Bump`-free, so no clone escapes the `Ctx`. The `Bump` is
// therefore never shared between threads and its `Rc` clones are
// never split across threads, so treating `Bump` as `Send + Sync`
// is sound for our usage.
ranges: unsafe { UnsafeSendSync::new(Default::default()) },
bundles: unsafe { UnsafeSendSync::new(Default::default()) },
scratch_bump: unsafe { UnsafeSendSync::new(Default::default()) },
vregs: unsafe { UnsafeSendSync::new(Default::default()) },

cfginfo: Default::default(),
cfginfo_ctx: Default::default(),
liveins: Default::default(),
liveouts: Default::default(),
blockparam_outs: Default::default(),
blockparam_ins: Default::default(),
spillsets: Default::default(),
pregs: Default::default(),
allocation_queue: Default::default(),
spilled_bundles: Default::default(),
spillslots: Default::default(),
slots_by_class: Default::default(),
extra_spillslots_by_class: Default::default(),
preferred_victim_by_class: Default::default(),
multi_fixed_reg_fixups: Default::default(),
allocated_bundle_count: Default::default(),
debug_annotations: Default::default(),
annotations_enabled: Default::default(),
conflict_set: Default::default(),
output: Default::default(),
scratch_conflicts: Default::default(),
scratch_bundle: Default::default(),
scratch_vreg_ranges: Default::default(),
scratch_spillset_pool: Default::default(),
scratch_workqueue: Default::default(),
scratch_operand_rewrites: Default::default(),
scratch_removed_lrs: Default::default(),
scratch_removed_lrs_vregs: Default::default(),
scratch_workqueue_set: Default::default(),
}
}
}

impl Ctx {
Expand Down
4 changes: 2 additions & 2 deletions src/ion/liveranges.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl<'a, F: Function> Env<'a, F> {
self.ctx.vregs.add(
VReg::new(idx, RegClass::Int),
VRegData {
ranges: LiveRangeList::new_in(self.ctx.bump()),
ranges: LiveRangeList::new_in(self.ctx.scratch_bump.clone()),
blockparam: Block::invalid(),
// We'll learn the RegClass as we scan the code.
class: None,
Expand Down Expand Up @@ -196,7 +196,7 @@ impl<'a, F: Function> Env<'a, F> {
{
// Is not contiguous with previously-added (immediately
// following) range; create a new range.
let lr = self.ctx.ranges.add(range, self.ctx.bump());
let lr = self.ctx.ranges.add(range, self.ctx.scratch_bump.clone());
self.ranges[lr].vreg = vreg;
self.vregs[vreg]
.ranges
Expand Down
2 changes: 1 addition & 1 deletion src/ion/merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ impl<'a, F: Function> Env<'a, F> {
continue;
}

let bundle = self.ctx.bundles.add(self.ctx.bump());
let bundle = self.ctx.bundles.add(self.ctx.scratch_bump.clone());
let mut range = self.vregs[vreg].ranges.first().unwrap().range;

self.bundles[bundle].ranges = self.vregs[vreg].ranges.clone();
Expand Down
19 changes: 11 additions & 8 deletions src/ion/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ impl<'a, F: Function> Env<'a, F> {
if idx.is_valid() {
Some(idx)
} else if create_if_absent {
let idx = self.ctx.bundles.add(self.ctx.bump());
let idx = self.ctx.bundles.add(self.ctx.scratch_bump.clone());
self.ctx.spillsets[ssidx].spill_bundle = idx;
self.ctx.bundles[idx].spillset = ssidx;
self.ctx.spilled_bundles.push(idx);
Expand Down Expand Up @@ -592,7 +592,7 @@ impl<'a, F: Function> Env<'a, F> {
from: split_at,
to: new_lr_list[0].range.to,
},
self.ctx.bump(),
self.ctx.scratch_bump.clone(),
);
self.ctx.ranges[new_lr].vreg = self.ranges[orig_lr].vreg;
trace!(" -> splitting LR {:?} into {:?}", orig_lr, new_lr);
Expand Down Expand Up @@ -634,7 +634,7 @@ impl<'a, F: Function> Env<'a, F> {
});
}

let new_bundle = self.ctx.bundles.add(self.ctx.bump());
let new_bundle = self.ctx.bundles.add(self.ctx.scratch_bump.clone());
trace!(" -> creating new bundle {:?}", new_bundle);
self.ctx.bundles[new_bundle].spillset = spillset;
for entry in &new_lr_list {
Expand Down Expand Up @@ -683,7 +683,7 @@ impl<'a, F: Function> Env<'a, F> {
from: split,
to: end,
};
let empty_lr = self.ctx.ranges.add(range, self.ctx.bump());
let empty_lr = self.ctx.ranges.add(range, self.ctx.scratch_bump.clone());
self.ctx.bundles[spill].ranges.push(LiveRangeListEntry {
range,
index: empty_lr,
Expand Down Expand Up @@ -748,7 +748,7 @@ impl<'a, F: Function> Env<'a, F> {
from: start,
to: split,
};
let empty_lr = self.ctx.ranges.add(range, self.ctx.bump());
let empty_lr = self.ctx.ranges.add(range, self.ctx.scratch_bump.clone());
self.ctx.bundles[spill].ranges.push(LiveRangeListEntry {
range,
index: empty_lr,
Expand Down Expand Up @@ -881,13 +881,13 @@ impl<'a, F: Function> Env<'a, F> {

// Create a new LR.
let cr = minimal_range_for_use(&u);
let lr = self.ctx.ranges.add(cr, self.ctx.bump());
let lr = self.ctx.ranges.add(cr, self.ctx.scratch_bump.clone());
new_lrs.push((vreg, lr));
self.ctx.ranges[lr].uses.push(u);
self.ctx.ranges[lr].vreg = vreg;

// Create a new bundle that contains only this LR.
let new_bundle = self.ctx.bundles.add(self.ctx.bump());
let new_bundle = self.ctx.bundles.add(self.ctx.scratch_bump.clone());
self.ctx.ranges[lr].bundle = new_bundle;
self.ctx.bundles[new_bundle].spillset = spillset;
self.ctx.bundles[new_bundle]
Expand Down Expand Up @@ -915,7 +915,10 @@ impl<'a, F: Function> Env<'a, F> {
// Make one entry in the spill bundle that covers the whole range.
// TODO: it might be worth tracking enough state to only create this LR when there is
// open space in the original LR.
let spill_lr = self.ctx.ranges.add(spill_range, self.ctx.bump());
let spill_lr = self
.ctx
.ranges
.add(spill_range, self.ctx.scratch_bump.clone());
self.ctx.ranges[spill_lr].vreg = vreg;
self.ctx.ranges[spill_lr].bundle = spill;
self.ctx.ranges[spill_lr].uses.extend(spill_uses.drain(..));
Expand Down
Loading