Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
prevent RVO and in-place construction via an MIR pass (#815)
## Summary Add a MIR pass that injects write-to-temporaries where necessary, which, if enabled, guarantees to the C code generator that it can always use RVO and in-place aggregate construction. This removes the dependency on `isPartOf` from `cgen`, makes the return-value and in-place-construction optimization related logic backend-agnostic, and also adds a basic framework for MIR passes. In theory, the VM code-generator could also use the in-place aggregate construction optimization now. In addition, multiple bugs where the in-place construction optimization was not correctly prevented when using the C backend are fixed: ```nim # in the following statements, the left-hand side is (in terms of # behaviour) now fully evaluated first (previously it wasn't) arr = [1, arr[0]] arr = [1, (let v = arr[0]; v)] arr = [1, call(arr[0])] obj = Obj(a: 1, b: (var v = obj.a; v)) # the following doesn't crash with an NPE anymore: obj = Obj(prc: ...) obj = Obj(prc: nil, other: obj.prc()) ``` ## Details In preparation for the introduction of more MIR passes, a dedicated module with the very basic framework is added. The only public routine is the `applyPasses` procedure, which runs all passes enabled for the specified backend. Since the targeted backend can be different from the one selected by the user (because of compile-time execution), a dedicated enum is used. Due to their similarity in processing, the temporary injection for calls and aggregate constructions is combined into a single pass. The pass works by searching for assignments where the source operand is the result of a call or aggregate construction and then running an analysis for whether the assignment destination is used in a way that prevents RVO or in-place construction. If the destination does, the source r- value is assigned to an intermediate temporary first. In the abstract, the used analysis works similar to `isPartOf`, but it is, apart from the type-analysis, implemented from scratch for the MIR. ### `transf` For the purpose of making assignments like `x = (x.a, x.b)` work when using the C backend, `transf` previously unpacked the literal tuple construction expression and then repacked it, like so: ```nim let a = x.a b = x.b x = (a, b) ``` While this does what is intended, it introduced an left-to-right evaluation-order violation when the `x` has side effects. The new MIR pass taking care of introducing a temporary allows for the removal of ``transformAsgn``, fixing the aforementioned issue and slightly reducing the amount of work for the `injectdestructors` pass (because there are less locals to analyze). The removal does have an unintended side-effect: r-value inputs to literal tuple construction expressions are not properly destroyed when an exception is raised (see the changed `ttuple_unpacking.nim` test). This is a known and documented issue for array and object constructions, which now also affects tuple constructions. ### `cgen` Calls that are the source operand to an assignment and return the result via an out parameter always use the assignment destination as their `Result` argument now; the `preventNrvo` procedure is renamed to `reportObservableStore` and the related injection of temporaries removed. In `genObjConstr`, a temporary now only has to be created for `ref`s and when the destination is an unset location. For literal `seq` construction expressions (e.g., `@[1, 2]`), in-place construction cannot be used anymore, since `isPartOf` usage is phased out and the new MIR pass doesn't special-case `mArrToSeq`. However, with the removal of the legacy GCs, the in-place `seq` construction had only very little benefit, as using a temporary there only implies an additional `(int, pointer)` local plus assignment thereof. ### Misc Two internal bugs in the `mirtree` module are fixed: - `operand` incorrectly ignored `mnkConsume` nodes - the `SingleInputNode` set was missing `mnkStdConv`, `mnkPathNamed`, `mnkPathPos`, and `mnkPathVariant`. The aforementioned nodes now work with `unaryOperand`, as they should
- Loading branch information
