feat(tvix/eval): deduplicate overlap between Closure and Thunk r/5159

This commit deduplicates the Thunk-like functionality from Closure
and unifies it with Thunk.

Specifically, we now have one and only one way of breaking reference
cycles in the Value-graph: Thunk.  No other variant contains a
RefCell.  This should make it easier to reason about the behavior of
the VM.  InnerClosure and UpvaluesCarrier are no longer necessary.

This refactoring allowed an improvement in code generation:
`Rc<RefCell<>>`s are now created only for closures which do not have
self-references or deferred upvalues, instead of for all closures.
OpClosure has been split into two separate opcodes:

- OpClosure creates non-recursive closures with no deferred
  upvalues.  The VM will not create an `Rc<RefCell<>>` when executing
  this instruction.

- OpThunkClosure is used for closures with self-references or
  deferred upvalues.  The VM will create a Thunk when executing this
  opcode, but the Thunk will start out already in the
  `ThunkRepr::Evaluated` state, rather than in the
  `ThunkRepr::Suspeneded` state.

To avoid confusion, OpThunk has been renamed OpThunkSuspended.

Thanks to @sterni for suggesting that all this could be done without
adding an additional variant to ThunkRepr.  This does however mean
that there will be mutating accesses to `ThunkRepr::Evaluated`,
which was not previously the case.  The field `is_finalised:bool`
has been added to `Closure` to ensure that these mutating accesses
are performed only on finalised Closures.  Both the check and the
field are present only if `#[cfg(debug_assertions)]`.

Change-Id: I04131501029772f30e28da8281d864427685097f
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>

1 files changed, 14 insertions, 12 deletions

diff --git a/tvix/eval/src/opcode.rs b/tvix/eval/src/opcode.rs
index 6b2783716cc8..382a857ef93d 100644
--- a/tvix/eval/src/opcode.rs
+++ b/tvix/eval/src/opcode.rs
@@ -144,25 +144,27 @@ pub enum OpCode {
     OpCall,
     OpTailCall,
     OpGetUpvalue(UpvalueIdx),
+    // A Closure which has upvalues but no self-references
     OpClosure(ConstantIdx),
-
-    // Thunks
-    OpThunk(ConstantIdx),
+    // A Closure which has self-references (direct or via upvalues)
+    OpThunkClosure(ConstantIdx),
+    // A suspended thunk, used to ensure laziness
+    OpThunkSuspended(ConstantIdx),
     OpForce,
 
     /// Finalise initialisation of the upvalues of the value in the
     /// given stack index after the scope is fully bound.
     OpFinalise(StackIdx),
 
-    // [`OpClosure`] and [`OpThunk`] have a variable number of
-    // arguments to the instruction, which is represented here by
-    // making their data part of the opcodes.  Each of these two
-    // opcodes has a `ConstantIdx`, which must reference a
-    // `Value::Blueprint(Lambda)`.  The `upvalue_count` field in
-    // that `Lambda` indicates the number of arguments it takes, and
-    // the `OpClosure` or `OpThunk` must be followed by exactly this
-    // number of `Data*` opcodes.  The VM skips over these by
-    // advancing the instruction pointer.
+    // [`OpClosure`], [`OpThunkSuspended`], and [`OpThunkClosure`] have a
+    // variable number of arguments to the instruction, which is
+    // represented here by making their data part of the opcodes.
+    // Each of these two opcodes has a `ConstantIdx`, which must
+    // reference a `Value::Blueprint(Lambda)`.  The `upvalue_count`
+    // field in that `Lambda` indicates the number of arguments it
+    // takes, and the opcode must be followed by exactly this number
+    // of `Data*` opcodes.  The VM skips over these by advancing the
+    // instruction pointer.
     //
     // It is illegal for a `Data*` opcode to appear anywhere else.
     DataLocalIdx(StackIdx),