mirpasses: make call-argument fixup a MIR pass

[zerbina]

Summary

Replace the PNode-based fixup for call arguments in ccgcalls with a
MIR pass. This removes a dependency on PNode-based analysis from
cgen, and also makes it possible to, in the future, enable the fixup
for the JS or VM backends.

While the used analysis stays mostly the same, an observable evaluation-
order violation is fixed. Injecting (shallow) copies for values passed
to both immutable and mutable parameters now considers all parameters,
instead of only parameters to the right of immutable ones. For example,
given:

f(a, a) # proc f(x: var T, y: T)

this fixes mutations through x inside f being visible on y.

Details

The analysis used by the MIR pass works mostly the same as the one
previously used in ccgcalls: for each argument value that is not
explicitly passed by-reference, it is analyzed whether:

• the value is potentially mutated after it is bound to the parameter
  but before the procedure is called
• the value is potentially also passed to a var parameter
  If either is the case, the argument is shallow-copied to a temporary
  that is then passed to the parameter instead.

The differences compared to the previous analysis are that:

• checking whether the argument value (or something that potentially
  overlaps with it in memory) is also passed to a mutable parameter
  now also considers preceding parameters. Previously, only the
  following parameters were checked
• testing for overlapping values considers the whole path now, instead
  of only the root. In effect, this means that for f(a.x, a.y), where
  the second parameter is mutable, no temporary is (unnecessarily)
  injected for the first parameter

For overlap testing, the maybeSameMutableLocation procedure is
introduced, which mirrors the behaviour of dfa.aliases (the routine
used by the previous PNode-based analysis). Since dereferences of
pointer-like values are treated like a normal field access, calls like
f(a[].x, b[].y), where one of the parameters is mutable and a and
b point to the same location, still cause observable evaluation-order
violations.

Finally, the analysis and temporary injection in ccgcalls is removed,
and a test is added for the fixed issue.

[zerbina]

I'm increasingly starting to think that generalizing the original mnkModify into mnkTag was a mistake. As the comment mentions, all current value tags represent some form of mutation, but the name mnkTag doesn't really convey that.

I'll think about it some more, but my current opinion is that mnkTag should become mnkMutation (or similar).

[saem]

unless the intention is to generalize across other effects, I agree I think a name indicating mutation/memory makes sense.

[saem]

very cool

[saem]

I'm increasingly convinced that destructors, as realized are a misfeature, or one that should be deemphasized. instead we should trigger ops on the containing type, which would be better for data oriented programming and allow for better composition. the op interface might be: =receiveCopy, =takeOwnership, =endOfLife, etc.

[Clyybber]

What does instead we should trigger ops on the containing type mean concretely, since the current hooks are attached to the respective type?

[saem]

type
  seq[T] = object # for discussion assume this definition
    data: uncheckedArray[T]
    len: int

proc `=takeOwnership`[T](s: seq[T], t: T) = # ... whatever default impl we want

# `seq` is a container and its 'containers ops' are triggered based on a `T`'s lifetime (like current type ops), but call `seq[T]`'s 'container ops'
# this also opens the door on specialization based on the contained type and overriding might be feasible

proc `=takeOwnership`(s: seq[Uri], u: Uri) = # ... specialized impl

I'm not sure about the actual events/hooks we want, but what I'm somewhat certain about is type ops triggered and called on the same type is 'off'. To clarify, Uri could have container ops of its own under this scheme, but they'd apply to the pile of string fields that are in its type definition and not the Uri type itself.

---

I had separately started thinking about triggered ops on containers before and I was coming at it from a DOD perspective. That further corroborated that type ops on the actual type aren't as useful wrt correctness or saving work, below is a snippet I was using to think about this.

Using the below example, if we specialized the Component types with type ops we'd end up with something "wrong". Those types would start having to know where and how they're stored, plus others couldn't reuse the components and associated systems easily (poor composition).

type
  Position* = object
    x*, y*: int64
  Movement* = object
    dx*, dy*, ddx*, ddy*: float32
  Component* = Position | Movement
  EntityData = object
  Store* = object
    allocator: Allocator
    entities*: seq[EntityData]
    positions*: seq[Position]
    movements*: seq[Movement]
proc `=alloc`[T=Component or EntitData](s: Store, t: typedesc[T])
proc `=dealloc`[T=Component or EntitData](s: Store, a: T)
proc `=transferInto`[T=Component or EntitData](s: Store, a: T)
proc `=copyInto`[T=Component or EntitData](s: Store, a: T)

The last bit I should add is that we effectively do this sort of logic for the stack, the push/pop of activation records, lifting of envs and associated allocs/deallocs, etc. All that is more call stack/container focused, that's what's being bumped, that's the ever present context. If we were actually modelling a call stack, we wouldn't want to put the type ops on each call frame type (that's a lot of object types 😆), we'd want activation record lifecycle events to trigger container ops on the call stack (container).

Anyhow, I hope that makes some sense.

[saem]

this gave me an idea, maybe we should add a doAssertKnownIssue that'll work with inverted behaviour.

[zerbina]

I like it, I've come across multiple cases were that would have been useful.

[saem]

/merge

[github-actions]

Merge requested by: @saem

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

---

Notes for Reviewers

• another preparation for compiler: introduce an IR for the code generators #551
• the new pass is not enabled for the JavaScript and VM backends yet