about summary refs log tree commit diff
path: root/tvix/eval/src/opcode.rs
AgeCommit message (Collapse)AuthorFilesLines
2024-08-19 r/8519 refactor(tvix/eval): ensure VM operations fit in a single byteVincent Ambo1-85/+192
This replaces the OpCode enum with a new Op enum which is guaranteed to fit in a single byte. Instead of carrying enum variants with data, every variant that has runtime data encodes it into the `Vec<u8>` that a `Chunk` now carries. This has several advantages: * Less stack space is required at runtime, and fewer allocations are required while compiling. * The OpCode doesn't need to carry "weird" special-cased data variants anymore. * It is faster (albeit, not by much). On my laptop, results consistently look approximately like this: Benchmark 1: ./before -E '(import <nixpkgs> {}).firefox.outPath' --log-level ERROR --no-warnings Time (mean ± σ): 8.224 s ± 0.272 s [User: 7.149 s, System: 0.688 s] Range (min … max): 7.759 s … 8.583 s 10 runs Benchmark 2: ./after -E '(import <nixpkgs> {}).firefox.outPath' --log-level ERROR --no-warnings Time (mean ± σ): 8.000 s ± 0.198 s [User: 7.036 s, System: 0.633 s] Range (min … max): 7.718 s … 8.334 s 10 runs See notes below for why the performance impact might be less than expected. * It is faster while at the same time dropping some optimisations we previously performed. This has several disadvantages: * The code is closer to how one would write it in C or Go. * Bit shifting! * There is (for now) slightly more code than before. On performance I have the following thoughts at the moment: In order to prepare for adding GC, there's a couple of places in Tvix where I'd like to fence off certain kinds of complexity (such as mutating bytecode, which, for various reaons, also has to be part of data that is subject to GC). With this change, we can drop optimisations like retroactively modifying existing bytecode and *still* achieve better performance than before. I believe that this is currently worth it to pave the way for changes that are more significant for performance. In general this also opens other avenues of optimisation: For example, we can profile which argument sizes actually exist and remove the copy overhead of varint decoding (which does show up in profiles) by using more adequately sized types for, e.g., constant indices. Known regressions: * Op::Constant is no longer printing its values in disassembly (this can be fixed, I just didn't get around to it, will do separately). Change-Id: Id9b3a4254623a45de03069dbdb70b8349e976743 Reviewed-on: https://cl.tvl.fyi/c/depot/+/12191 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-12-29 r/7272 refactor(tvix/eval): let OpCoerceToString select the CoercionKindAdam Joseph1-4/+2
Change-Id: I92d58ef216d7e0766af70f019b3dcd445284a95d Reviewed-on: https://cl.tvl.fyi/c/depot/+/10344 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-14 r/7218 fix(tvix/eval): remove incorrect imports when coercingsterni1-1/+2
The default behavior of string coercion in C++ Nix is to weakly coerce and import to store if necessary. There is a flag to make it strongly coerce (coerceMore) and a flag that controls whether path values have the corresponding file/directory imported into the store before returning the (store) path as a string (copyToStore). We need to implement our equivalent to the copyToStore (import_paths) flag for the benefit of weak coercions that don't import into the store (dirOf, baseNameOf, readFile, ...) and strong coercions that don't import into the store (toString). This makes coerce_to_string as well as CoercionKind weirder and more versatile, but prevents us from reimplementing parts of the coercion logic constantly as can be seen in the case of baseNameOf. Note that it is not possible to test this properly in //tvix/eval tests due to the lack of an appropriate EvalIO implementation being available. Tests should be added to //tvix/glue down the line. Change-Id: I8fb8ab99c7fe08e311d2ba1c36960746bf22f566 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10361 Autosubmit: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2023-12-12 r/7178 fix(tvix/eval): fix branching on catchable defaults (b/343)Adam Joseph1-0/+6
This commit adds Opcode::OpJumpIfCatchable, which can be inserted ahead of most VM operations which expect a boolean on the stack, in order to handle catchables in branching position properly. Other than remembering to patch the jump, no other changes should be required. This commit also fixes b/343 by emitting this new opcode when compiling if-then-else. There are probably other places where we need to do the same thing. Change-Id: I48de3010014c0bbeba15d34fc0d4800e0bb5a1ef Reviewed-on: https://cl.tvl.fyi/c/depot/+/10288 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-09-08 r/6571 docs(tvix/eval): document remaining opcodesVincent Ambo1-3/+48
Change-Id: Iad8a5f78930872719b6481ddf1bbad8532bfa888 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7981 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2023-09-08 r/6570 docs(tvix/eval): document attribute set related opcodesVincent Ambo1-0/+11
Change-Id: Ib98a9fe8c9aa3f0e61c27d10285c0926cda7a969 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7979 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-09-08 r/6569 docs(tvix/eval): add documentation strings for some OpCode variantsVincent Ambo1-2/+37
Change-Id: I42e610740b3687e1fd897d36694cce425751a8bc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7975 Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de>
2023-06-20 r/6336 fix(tvix/eval): only finalise formal arguments if defaultingsterni1-0/+1
When dealing with a formal argument in a function argument pattern that has a default expression, there are two different things that can happen at runtime: Either we select its value from the passed attribute successfully or we need to use the default expression. Both of these may be thunks and both of these may need finalisers. However, in the former case this is taken care of elsewhere, the value will always be finalised already if necessary. In the latter case we may need to finalise the thunk resulting from the default expression. However, the thunk corresponding to the expression may never end up in the local's stack slot. Since finalisation goes by stack slot (and not constants), we need to prevent a case where we don't fall back to the default expression, but finalise anyways. Previously, we worked around this by making `OpFinalise` ignore non-thunks. Since finalisation of already evaluated thunks still crashed, the faulty compilation of function pattern arguments could still cause a crash. As a new approach, we reinstate the old behavior of `OpFinalise` to crash whenever encountering something that is either not a thunk or doesn't need finalisation. This can also help catching (similar) miscompilations in the future. To then prevent the crash, we need to track whether we have fallen back or not at runtime. This is done using an additional phantom on the stack that holds a new `FinaliseRequest` value. When it comes to finalisation we check this value and conditionally execute `OpFinalise` based on its value. Resolves b/261 and b/265 (partially). Change-Id: Ic04fb80ec671a2ba11fa645090769c335fb7f58b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8705 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org>
2023-06-07 r/6243 fix(tvix/eval): type check function argument with set patternsterni1-0/+1
C++ Nix forces and typechecks the passed argument even if it is not necessary in order to compute the return value of the function. I discovered this when I thought our formals miscompilation might be that we are too strict, but doesn't look like it in this case. Change-Id: Ifb3c92592293052c489d1e3ae8c7c54e4b6b4dc6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8701 Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su>
2023-03-13 r/5964 refactor(tvix/eval): flatten call stack of VM using generatorsVincent Ambo1-0/+8
Warning: This is probably the biggest refactor in tvix-eval history, so far. This replaces all instances of trampolines and recursion during evaluation of the VM loop with generators. A generator is an asynchronous function that can be suspended to yield a message (in our case, vm::generators::GeneratorRequest) and receive a response (vm::generators::GeneratorResponsee). The `genawaiter` crate provides an interpreter for generators that can drive their execution and lets us move control flow between the VM and suspended generators. To do this, massive changes have occured basically everywhere in the code. On a high-level: 1. The VM is now organised around a frame stack. A frame is either a call frame (execution of Tvix bytecode) or a generator frame (a running or suspended generator). The VM has an outer loop that pops a frame off the frame stack, and then enters an inner loop either driving the execution of the bytecode or the execution of a generator. Both types of frames have several branches that can result in the frame re-enqueuing itself, and enqueuing some other work (in the form of a different frame) on top of itself. The VM will eventually resume the frame when everything "above" it has been suspended. In this way, the VM's new frame stack takes over much of the work that was previously achieved by recursion. 2. All methods previously taking a VM have been refactored into async functions that instead emit/receive generator messages for communication with the VM. Notably, this includes *all* builtins. This has had some other effects: - Some test have been removed or commented out, either because they tested code that was mostly already dead (nix_eq) or because they now require generator scaffolding which we do not have in place for tests (yet). - Because generator functions are technically async (though no async IO is involved), we lose the ability to use much of the Rust standard library e.g. in builtins. This has led to many algorithms being unrolled into iterative versions instead of iterator combinations, and things like sorting had to be implemented from scratch. - Many call sites that previously saw a `Result<..., ErrorKind>` bubble up now only see the result value, as the error handling is encapsulated within the generator loop. This reduces number of places inside of builtin implementations where error context can be attached to calls that can fail. Currently what we gain in this tradeoff is significantly more detailed span information (which we still need to bubble up, this commit does not change the error display). We'll need to do some analysis later of how useful the errors turn out to be and potentially introduce some methods for attaching context to a generator frame again. This change is very difficult to do in stages, as it is very much an "all or nothing" change that affects huge parts of the codebase. I've tried to isolate changes that can be isolated into the parent CLs of this one, but this change is still quite difficult to wrap one's mind and I'm available to discuss it and explain things to any reviewer. Fixes: b/238, b/237, b/251 and potentially others. Change-Id: I39244163ff5bbecd169fe7b274df19262b515699 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8104 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-03-07 r/5898 refactor(tvix/eval): merge OpCall & OpTailCallVincent Ambo1-1/+0
As applies are thunked, there was no situation where OpCall could be emitted. In practice, all calls were already tail calls. Change-Id: Id0d441dcdd86f804d7cddd0cc14f589bbfc75e5b Reviewed-on: https://cl.tvl.fyi/c/depot/+/8147 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-01-21 r/5721 refactor(tvix/eval): administer antidote for poisonAdam Joseph1-5/+0
The codebase contains a lot of complexity and odd roundabout handling for shadowing globals. I'm pretty sure none of this is necessary, and all of it disappears if you simply make the globals part of the ordinary identifier resolution chain, with their own scope up above the root scope. Then the ordinary shadowing routines do the right thing, and no special cases or new terminology are required. This commit does that. Note by tazjin: This commit was originally abandoned when Adam decided not to take away reviewer bandwidth for this at the time (eval was still in a much earlier stage). As we've recently done some significant refactoring of globals initialisation this came up again, and it seems we can easily cover the use-cases of the poison tracking in other ways now, so I've rebased, updated and resurrected the CL. Co-Authored-By: Vincent Ambo <tazjin@tvl.su> Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: Ib3309a47a7b31fa5bf10466bade0d876b76ae462 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7089 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2022-11-05 r/5252 fix(tvix/eval): change ordinary (//) to rustdoc-comments (///)Adam Joseph1-3/+3
This fixes a mistake I made in d978b556e6. Change-Id: I88db697105a7149e9785f6aface03bff68566d2b Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7085 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-11-05 r/5250 refactor(tvix/eval): rename Opcode::DataLocalIdx to DataStackIdxAdam Joseph1-1/+1
It is very confusing that this opcode is called DataLocalIdx, but it carries a StackIdx rather than a LocalIdx. It seems like this really ought to be called DataStackIdx, but maybe I've misunderstood; if so please explain it to me. Change-Id: I91f6ffa759412beef0b91d3c19ec0d873fe51b99 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7088 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-11-04 r/5245 docs(tvix/eval): add comments for Opcode::DataXXX opcodesAdam Joseph1-2/+8
Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: I8c72405880a9342eb502d92c1e0087f5bb17e03c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7087 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-26 r/5201 docs(tvix/eval): StackIdx, LocalIdx UpvalueIdxAdam Joseph1-2/+6
This adds a comment noting that StackIdx is an offset relative to the base of the current CallFrame, whereas UpvalueIdx is an absolute index into the upvalues array. It also removes the confusing mention of StackIdx in the descriptive comment for LocalIdx. They index into totally different structures; one exists at runtime and the other exists at compile time. Change-Id: Ib932b1b0679734c15001e8c5c95a08293fa016b4 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7017 Reviewed-by: grfn <grfn@gws.fyi> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-19 r/5159 feat(tvix/eval): deduplicate overlap between Closure and ThunkAdam Joseph1-12/+14
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>
2022-10-17 r/5154 feat(tvix/eval): Validate closed formalsGriffin Smith1-0/+6
Validate "closed formals" (formal parameters without an ellipsis) via a new ValidateClosedFormals op, which checks the arguments (in an attr set at the top of the stack) against the formal parameters on the Lambda in the current frame, and returns a new UnexpectedArgument error (including the span of the formals themselves!!) if any arguments aren't allowed Change-Id: Idcc47a59167a83be1832a6229f137d84e426c56c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7002 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-17 r/5151 docs(tvix/eval) comments for various fieldsAdam Joseph1-5/+12
Change-Id: I8dcddf2b419761e475e71215c199eef2f7dc61dc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7028 Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-16 r/5147 fix(tvix/eval): resolve home relative paths at runtimesterni1-0/+3
Home relative paths depend on the environment to be resolved. We have elected to do everything that depends on the environment, e.g. resolving SPATH expressions using NIX_PATH, at runtime, so tvix evaluation would continue to behave correctly even if we separated the compilation and execution phases more, e.g. via serializing bytecode. Then the value of HOME, NIX_PATH etc. could reasonably change in the time until execution, yielding wrong results if the resolution results were cached in the bytecode. We also take the opportunity to fix the broken path concatenation previously found in the compiler, fixing b/205. Another thing we could consider is emitting a warning for home relative path literals, as they are by nature relatively fragile. One sideeffect of this change is that home path resolution errors become catchable which is not the case in C++ Nix. This will need to be fixed up in a subsequent change. Change-Id: I30bd69b575667c49170a9fdea23a020565d0f9ec Reviewed-on: https://cl.tvl.fyi/c/depot/+/7024 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2022-10-14 r/5128 refactor(tvix/eval): remove OpResolveWithOrUpvalueAdam Joseph1-1/+0
The case branch in vm.rs for OpResolveWithOrUpvalue is unreachable/deadcode. I believe this opcode is unnecessary, since it should always be statically detectable (at parse-time) whether a reference is to an upvalue (i.e. enclosing binding); otherwise, and only then, is with-resolution applicable. Perhaps I've misunderstood how with-resolution works. If so, please explain it to me and -1/-2 this CL. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: I4a90b9eb6cb3396df92a6a943d42ecc301871ba0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7009 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-12 r/5113 refactor(tvix/eval) s/NixPath/NixSearchPath/Adam Joseph1-2/+2
Since NixString is the Rust type for nix strings, people might mistake NixPath for the Rust type of nix paths, which it is not. Let's call it NixSearchPath instead. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: Ib2ea155c4b27cb90d6180a04ea7b951d86607373 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6927 Reviewed-by: kanepyork <rikingcoding@gmail.com> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-10 r/5087 feat(tvix/eval): Initial resolution of `<...>` pathsGriffin Smith1-0/+6
This commit implements (lazy) resolution of `<...>` paths via either the NIX_PATH environment variable, or the -I command-line flag - both handled via EvalOptions. As a result, EvalOptions can no longer derive Copy, meaning we have to clone it at each line of the repl - this is probably not a huge deal as repl performance is not exactly an inner loop and we're not cloning very much. Internally, this works by creating a thunk which pushes a constant containing the string inside the brackets to the stack, then a new opcode to resolve that path via the `NixPath`. To get that opcode to work, we now have to pass in the NixPath when constructing the VM. This (intentionally) leaves out proper implementation of path resolution via `findFile` (cppnix just calls whatever identifier called findFile is in scope!!!) as that's widely considered a bit of a misfeature, but if we do decide to implement that down the road it likely wouldn't be more than a few extra ops within the thunk introduced here. Change-Id: Ibc979b7e425b65cbe88599940520239a4a10cee2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6918 Autosubmit: grfn <grfn@gws.fyi> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-10 r/5082 refactor(tvix/eval): Compile OpAssert using conditional jumpsGriffin Smith1-2/+2
In order to behave nicely with tryEval, asserts need to leave the instruction pointer in a reasonable place even if they fail - whereas with the previous implementation catching a failed assert would still end up running the op for the *body* of the assert. With this change, we compile asserts much more like an `if` expression with conditional jumps rather than having an OpAssert op. Change-Id: I1b266c3be90185c84000da6b1995ac3e6fd5471b Reviewed-on: https://cl.tvl.fyi/c/depot/+/6925 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-09-29 r/4990 chore(tvix/eval): remove existing nested key implementationVincent Ambo1-1/+0
This implementation, which only ever worked for non-recursive attribute sets, is no longer needed and thus removed here. We have a new implementation of these nested keys coming up instead. Change-Id: I0c2875154026a4f5f6e0aa038e465f54444bf721 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6783 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-18 r/4912 docs(tvix/eval): Document OpAttrsGriffin Smith1-0/+4
Document the OpAttrs op, paying special attention to the (perhaps confusing) behavior of taking the number of *pairs*, not the number of *values*, which will be popped off the stack into the resulting attr set. Change-Id: I64df0290308ecae7a5c7e14ead37091d32701507 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6654 Autosubmit: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su>
2022-09-18 r/4901 test(tvix/eval): Add proof-of-concept test for ChunkGriffin Smith1-4/+4
This is pretty boring at the moment, but mostly serves as a foot in the door in the direction of writing more tests Change-Id: Id88eb4ec7e53ebb2d5b5c254c8f45ff750238811 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6637 Autosubmit: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-09-17 r/4890 refactor(tvix/eval): rename OpAttrsIsSet -> OpHasAttrVincent Ambo1-1/+1
This matches the name of the AST node from which it was compiled. Suggested by sterni in cl/6231 Change-Id: Ia51525158d2f47467c01fce2282005b1a8417a47 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6623 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: grfn <grfn@gws.fyi>
2022-09-15 r/4860 fix(tvix/eval): coerce string interpolation parts to stringsterni1-0/+3
With this puzzle piece of string compilation in place, `compile_str` becomes less redundant, as every part now needs to be compiled the same. The thunking logic becomes a bit trickier, since we need to thunk even in the case of `count == 1` if the single part is interpolating. Splitting the inner (shared) code in a separate function turned out to be easier for making rustc content. Change-Id: I6a554ca599926ae5907d7acffce349c9616f568f Reviewed-on: https://cl.tvl.fyi/c/depot/+/6582 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-09-13 r/4838 refactor(tvix/eval): use CodeIdx wrapper for instruction pointerVincent Ambo1-0/+16
As suggested by sterni in cl/6453. Change-Id: I3cf80d97c11fd7d085ab510f6be4b5f937c791ec Reviewed-on: https://cl.tvl.fyi/c/depot/+/6562 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-11 r/4801 refactor(tvix/eval): capture entire with_stack in upvaluesVincent Ambo1-2/+1
This completely rewrites the handling of "dynamic upvalues" to, instead of resolving them at thunk/closure instantiation time (which forces some values too early), capture the entire with stack of parent contexts if it exists. There are a couple of things in here that could be written more efficiently, but I'm first working through this to get to a bug related to with + recursion and the code complexity of some of the optimisations is distracting. Change-Id: Ia538e06c9146e3bf8decb9adf02dd726d2c651cf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6486 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-10 r/4786 fix(tvix/eval): fix doc comment syntax where applicableVincent Ambo1-5/+5
As pointed out by grfn on cl/6091 Change-Id: I28308577b7cf99dffb4a4fd3cc8783eb9ab4d0d6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6460 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-10 r/4783 feat(tvix/eval): implement OpTailCallVincent Ambo1-0/+1
If the last operation within a chunk is a function call, the call can be executed in the same call frame without increasing the depth of the call stack. To enable this, a new OpTailCall instruction (similar to OpCall) is introduced, but not yet emitted by the compiler. Change-Id: I9ffbd7da6d2d6a8ec7a724646435dc6ee89712f2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6457 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-08 r/4756 fix(tvix/eval): compare *stack* slots when deciding whether to deferVincent Ambo1-1/+1
When deciding whether an upvalue needs to have a deferred resolution step, the *stack* indexes should be compared - not the locals indexes. The results are almost always the same, but there are tricky situations where this can cause errors. It's difficult to reproduce these errors in isolation, as they depend on other scope behaviour, so this is one in a series of commits to address the combination of issues which will gain some tests at the end. Change-Id: Iaa400b8d9500af58f493ab10e4f95022f3b5dd21 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6423 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-07 r/4706 fix(tvix/eval): address current clippy lintsVincent Ambo1-2/+2
Note that I've allowed `needless_lifetimes` for the attribute set iterator, as I find the type easier to understand with these annotations present. Change-Id: I33abb17837ee4813076cdb9a87f54bac4a37044e Reviewed-on: https://cl.tvl.fyi/c/depot/+/6373 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-07 r/4688 feat(tvix/eval): implement OpForce in VMVincent Ambo1-0/+1
This operation forces the evaluation of a thunk. There is some potential here for making an implementation that avoids some copies, but the thunk machinery is tricky to get right so the first priority is to make sure it is correct by keeping the implementation simple. Change-Id: Ib381455b02f42ded717faff63f55afed4c8fb7e3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6352 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-06 r/4674 feat(tvix/eval): Add Compiler::thunk method for emitting thunksVincent Ambo1-0/+3
The logic in this method is *very* similar to `compile_lambda`. It is intended to be called around any expression that should be thunked (such as function applications, attribute set values, etc.). Change-Id: Idfbb2daa9f4b735095378fb9c39a2fd07c8cff91 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6344 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-06 r/4667 feat(tvix/eval): implement OpFinalise instructionVincent Ambo1-1/+4
This instruction finalises the initialisation of deferred upvalues in closures (and soon, thunks). The compiler does not yet emit this instruction, some more accounting is needed for that. Change-Id: Ic4181b26e19779e206f51e17388559400da5f93a Reviewed-on: https://cl.tvl.fyi/c/depot/+/6337 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-06 r/4665 feat(tvix/eval): detect deferred upvalue capturingVincent Ambo1-0/+1
Uses the threaded through slot offset to determine whether initialisation of a captured local upvalue must be defered to a later point where all values of a scope are available. This adds a new data representation to the opcode for this situation, but the equivalent runtime handling is not yet implemented. This is in part because there is more compiler machinery needed to find the resolution point. Change-Id: Ifd0c393f76abfe6e2d91483faf0f58947ab1dedc Reviewed-on: https://cl.tvl.fyi/c/depot/+/6329 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-06 r/4658 fix(tvix/eval): correctly thread through dynamic upvaluesVincent Ambo1-0/+2
This puts together the puzzle pieces for threading dynamic upvalues (that is, upvalues resolved from the `with`-stack) all the way through. Reading the test case enclosed in this commit and walking through it is recommended to understand what problem is being tackled here. In short, because the compiler can not statically know *which* with-scope a dynamic argument is resolved from it needs to lay the groundwork for resolving from *all* possible scopes. There are multiple different approaches to doing this. The approach chosen in this commit is that if a dynamic upvalue is detected, the compiler will emit instructions to close over this dynamic value in *all* enclosing lambda contexts. It uses a new instruction for this that will leave around a sentinel value in case an identifier could not be resolved, and wire the location of this found value (or sentinel) up through the upvalues to the next level of nesting. In this tradeoff, tvix potentially closes over more upvalues than are needed (but in practice, how often do people create *really* deep `with`-stacks? and in *this* kind of code situation? maybe we should even warn for this!) but avoids keeping the entire attribute sets themselves around. Looking at the test case, each surrounding closure will close over *all* dynamic identifiers that are referenced later on visible to it, but only the last one for each identifier will actually end up being used. This also covers our bases for an additional edge-case this creates, in which an identifier potentially resolves to a dynamic upvalue *and* to a dynamic value within the function's own scope (again, would anyone really do this?) by introducing a resolution instruction for that particular case. There is likely some potential for cleaning up this code which is quite ugly in some parts, but as this implementation is now carefully calibrated to work I decided it is time to commit it and clean it up in subsequent commits. Change-Id: Ib701e3e6da39bd2c95938d1384036ff4f9fb3749 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6322 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-04 r/4635 feat(tvix/eval): implement upvalue resolution in `with` scopesVincent Ambo1-0/+1
These need to be handled specially by the runtime if the compiler determines that a given local must be resolved via `with`. Note that this implementation has a bug: It currently allows `with` inside of nested lambdas to shadow statically known identifiers. This will be cleaned up in the next commit. Change-Id: If196b99cbd1a0f2dbb4a40a0e88cdb09a009c6b9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6299 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-03 r/4625 feat(tvix/eval): compile creation of closure objectsVincent Ambo1-1/+11
Fully implements the instructions for compiling closure objects (without runtime handling yet). Closure (and thunk) objects are created at runtime by capturing all known upvalues. To represent this, the instructions for creating them need to have a variable number of arguments. Due to that, this commit introduces new variants in OpCode that are not actually operations, but data. If the VM is implemented correctly, the instruction pointer should never point at these. Due to this, the VM will panic if it sees a data operand during an execution run. Change-Id: Ic56b49b3a42736dc437751e76df0e89c8d0619c6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6291 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-09-03 r/4623 feat(tvix/eval): implement compilation of upvalue accessVincent Ambo1-1/+7
This adds a new upvalue tracking structure in the compiler to resolve upvalues and track their positions within a function when compiling a closure. The compiler will emit runtime upvalue access instructions after this commit, but the creation of the runtime closure object etc. is not yet wired up. Change-Id: Ib0c2c25f686bfd45f797c528753068858e3a770d Reviewed-on: https://cl.tvl.fyi/c/depot/+/6289 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-09-03 r/4622 refactor(tvix/eval): add opcode::Count type for less ambiguityVincent Ambo1-5/+11
Change-Id: Ibde0b2baa1128a74c1364ee9a6330b62db3da699 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6288 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-03 r/4621 refactor(tvix/eval): add opcode::StackIdx type for less ambiguityVincent Ambo1-2/+7
Change-Id: I9b9de1f681972c205d4d20bc5731d2ce79858edb Reviewed-on: https://cl.tvl.fyi/c/depot/+/6287 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-03 r/4620 docs(tvix/eval): add docstrings for usize wrappers in opcodeVincent Ambo1-0/+3
Change-Id: I11b9324233c0aa48bd2fbac15a484962f925e72e Reviewed-on: https://cl.tvl.fyi/c/depot/+/6283 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-03 r/4619 refactor(tvix/eval): add opcode::JumpOffset type for less ambiguityVincent Ambo1-4/+10
This adds a transparent wrapper around `usize` used for jump offsets in the opcodes. This is a step towards getting rid of ambiguous plain `usize` usage in the opcode. Change-Id: I21e35e67d94b32d68251908b96c7f62b6f56a8bb Reviewed-on: https://cl.tvl.fyi/c/depot/+/6282 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-03 r/4613 refactor(tvix/eval): rename Value::NotFound & OpAttrOrNotFoundVincent Ambo1-1/+1
grfn suggested clearer naming for these in cl/6166. Change-Id: I83164bf1d1902ebd42272e9d5d63819a0f6a72c5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6277 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-09-03 r/4608 fix(tvix/eval): address all current clippy lintsVincent Ambo1-0/+1
Change-Id: I758fc4f3b9078de7ca6228a75a4351c3e085c4cf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6272 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-09-02 r/4583 feat(tvix/eval): implement opcode for function calls in VMVincent Ambo1-0/+3
Nix functions always have a single argument and we do not yet make efforts to optimise this in Tvix for known multi-argument functions being directly applied. For this reason, the call instruction is fairly simple and just calls out to construct a new call frame. Note that the logic for terminating the run loop has moved to the top of the dispatch; this is because the loop run needs to be skipped if the call frame for the current lambda has just been dropped. Change-Id: I259bc07e19c1e55cd0a65207fa8105b23052b967 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6249 Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI