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

prevent RVO and in-place construction via an MIR pass #815

Merged
merged 11 commits into from
Jul 28, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jul 26, 2023

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:

  # 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:

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 refs 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

`mnkConsume` nodes were incorrectly ignored, leading to the
`unreachable` statement being erroneously triggered.
It's purpose was to prevent problematic in-place constructions, but the
logic is obsolete now with the introduction of the MIR pass.
This is the responsibility of the new MIR pass now.
A temporary is now injected by the MIR pass, meaning that a
`genObjConstr` must only create one for refs and when there's no
destination.

In-place construction for seq constructions (e.g., `a = @[1, 2]`) is
removed. It had little to no benefit (assigning the sequence is just a
pointer + int assignment).
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Jul 26, 2023
@zerbina zerbina added this to the MIR phase milestone Jul 26, 2023
@zerbina zerbina marked this pull request as draft July 27, 2023 14:48
@zerbina
Copy link
Collaborator Author

zerbina commented Jul 27, 2023

This is not fit for merging yet -- I've found some implementation bugs that need to be fixed first. The PR is still ready for review, however.

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Only found some doc typos, they aren't blockers.

The tests are nice and thorough, the new pass seems tighter and easier to follow.

compiler/mir/mirpasses.nim Outdated Show resolved Hide resolved
tests/assign/tin_place_construction.nim Outdated Show resolved Hide resolved
The path operator were still missing.
The set was missing various operators, and the documentation was missing
a note about only the immediate operator being looked at.

While `mnkNone` isn't really something that produces an r-value,
including it here makes handling `mnkNone` arguments easier.
The set of allowed nodes was missing a lot of nodes. Empty arg-blocks
weren't properly considered, too.
@zerbina zerbina marked this pull request as ready for review July 27, 2023 22:20
@zerbina
Copy link
Collaborator Author

zerbina commented Jul 27, 2023

Thank you for the review, @saem! The logic unfortunately ended up more complex than I had hoped, but I think that this can be alleviated/remedied through some adjustments of the MIR's design.

I've fixed the issues I mentioned I had noticed. They were mostly with sets missing some node kinds.

@zerbina
Copy link
Collaborator Author

zerbina commented Jul 28, 2023

/merge

@github-actions
Copy link

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • removing usage of isPartOf in cgen is a preparation for compiler: introduce an IR for the code generators #551
  • many issues being fixed by rewriting/rethinking something from scratch is a re-occurring theme with the compiler
  • there are multiple issues with the MIR as it is today, but my plan is to first gather some implementation experience with the MIR passes before attempting to fix them

@chore-runner chore-runner bot added this pull request to the merge queue Jul 28, 2023
Merged via the queue into nim-works:devel with commit 26178e4 Jul 28, 2023
27 checks passed
@zerbina zerbina deleted the rvo-prevention-as-mir-pass branch July 29, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants