Skip to content

Commit

Permalink
fixes #19291; implements wasMoved hook (#21303)
Browse files Browse the repository at this point in the history
* fixes #19291; implements `wasMoved` hook

* basics

* checkpoint

* finish `wasMoved`

* add a test for #19291

* add documentation and changelog

* work `attachedWasMoved` with generics

* fixes optimizer

* register `=wasMoved`

* handle wasMoved magcis

* check another round

* some patches

* try `op == nil`

* nicer

* generate `wasMoved` before `destroy`

* try again

* fixes tests

* default wasMoved

* Update tests/destructor/tv2_cast.nim

* Update tests/destructor/tv2_cast.nim

* Update tests/arc/topt_refcursors.nim
  • Loading branch information
ringabout authored Mar 2, 2023
1 parent 9948fed commit a137e50
Show file tree
Hide file tree
Showing 12 changed files with 131 additions and 39 deletions.
2 changes: 2 additions & 0 deletions changelogs/changelog_2_0_0.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,8 @@

- IBM Z architecture and macOS m1 arm64 architecture are supported.

- `=wasMoved` can be overridden by users.

## Compiler changes

- The `gc` switch has been renamed to `mm` ("memory management") in order to reflect the
Expand Down
3 changes: 2 additions & 1 deletion compiler/ast.nim
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,7 @@ type
TTypeSeq* = seq[PType]

TTypeAttachedOp* = enum ## as usual, order is important here
attachedWasMoved,
attachedDestructor,
attachedAsgn,
attachedSink,
Expand Down Expand Up @@ -1502,7 +1503,7 @@ proc newProcNode*(kind: TNodeKind, info: TLineInfo, body: PNode,

const
AttachedOpToStr*: array[TTypeAttachedOp, string] = [
"=destroy", "=copy", "=sink", "=trace", "=deepcopy"]
"=wasMoved", "=destroy", "=copy", "=sink", "=trace", "=deepcopy"]

proc `$`*(s: PSym): string =
if s != nil:
Expand Down
17 changes: 12 additions & 5 deletions compiler/injectdestructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -354,11 +354,18 @@ It is best to factor out piece of object that needs custom destructor into separ
result.add newTree(nkFastAsgn, le, tmp)

proc genWasMoved(c: var Con, n: PNode): PNode =
result = newNodeI(nkCall, n.info)
result.add(newSymNode(createMagic(c.graph, c.idgen, "wasMoved", mWasMoved)))
result.add copyTree(n) #mWasMoved does not take the address
#if n.kind != nkSym:
# message(c.graph.config, n.info, warnUser, "wasMoved(" & $n & ")")
let typ = n.typ.skipTypes({tyGenericInst, tyAlias, tySink})
let op = getAttachedOp(c.graph, n.typ, attachedWasMoved)
if op != nil:
if sfError in op.flags:
c.checkForErrorPragma(n.typ, n, "=wasMoved")
result = genOp(c, op, n)
else:
result = newNodeI(nkCall, n.info)
result.add(newSymNode(createMagic(c.graph, c.idgen, "wasMoved", mWasMoved)))
result.add copyTree(n) #mWasMoved does not take the address
#if n.kind != nkSym:
# message(c.graph.config, n.info, warnUser, "wasMoved(" & $n & ")")

proc genDefaultCall(t: PType; c: Con; info: TLineInfo): PNode =
result = newNodeI(nkCall, info)
Expand Down
41 changes: 36 additions & 5 deletions compiler/liftdestructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ proc defaultOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
let call = genBuiltin(c, mDefault, "default", x)
call.typ = t
body.add newAsgnStmt(x, call)
elif c.kind == attachedWasMoved:
body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc genAddr(c: var TLiftCtx; x: PNode): PNode =
if x.kind == nkHiddenDeref:
Expand Down Expand Up @@ -145,6 +147,11 @@ proc destructorCall(c: var TLiftCtx; op: PSym; x: PNode): PNode =
else:
result = destroy

proc genWasMovedCall(c: var TLiftCtx; op: PSym; x: PNode): PNode =
result = newNodeIT(nkCall, x.info, op.typ[0])
result.add(newSymNode(op))
result.add genAddr(c, x)

proc fillBodyObj(c: var TLiftCtx; n, body, x, y: PNode; enforceDefaultOp: bool) =
case n.kind
of nkSym:
Expand Down Expand Up @@ -442,6 +449,20 @@ proc considerUserDefinedOp(c: var TLiftCtx; t: PType; body, x, y: PNode): bool =
body.add newDeepCopyCall(c, op, x, y)
result = true

of attachedWasMoved:
var op = getAttachedOp(c.g, t, attachedWasMoved)
if op != nil and sfOverriden in op.flags:

if op.ast.isGenericRoutine:
# patch generic destructor:
op = instantiateGeneric(c, op, t, t.typeInst)
setAttachedOp(c.g, c.idgen.module, t, attachedWasMoved, op)

#markUsed(c.g.config, c.info, op, c.g.usageSym)
onUse(c.info, op)
body.add genWasMovedCall(c, op, x)
result = true

proc declareCounter(c: var TLiftCtx; body: PNode; first: BiggestInt): PNode =
var temp = newSym(skTemp, getIdent(c.g.cache, lowerings.genPrefix), nextSymId(c.idgen), c.fn, c.info)
temp.typ = getSysType(c.g, body.info, tyInt)
Expand Down Expand Up @@ -524,6 +545,7 @@ proc fillSeqOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
if canFormAcycle(t.elemType):
# follow all elements:
forallElements(c, t, body, x, y)
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
createTypeBoundOps(c.g, c.c, t, body.info, c.idgen)
Expand Down Expand Up @@ -561,6 +583,7 @@ proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
if op == nil:
return # protect from recursion
body.add newHookCall(c, op, x, y)
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc fillStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
case c.kind
Expand All @@ -576,6 +599,7 @@ proc fillStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.add genBuiltin(c, mDestroy, "destroy", x)
of attachedTrace:
discard "strings are atomic and have no inner elements that are to trace"
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc cyclicType*(t: PType): bool =
case t.kind
Expand Down Expand Up @@ -674,6 +698,8 @@ proc atomicRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
# If the ref is polymorphic we have to account for this
body.add callCodegenProc(c.g, "nimTraceRefDyn", c.info, genAddrOf(x, c.idgen), y)
#echo "can follow ", elemType, " static ", isFinal(elemType)
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)


proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
## Closures are really like refs except they always use a virtual destructor
Expand Down Expand Up @@ -722,6 +748,7 @@ proc atomicClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace:
body.add callCodegenProc(c.g, "nimTraceRefDyn", c.info, genAddrOf(xenv, c.idgen), y)
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
case c.kind
Expand All @@ -746,6 +773,7 @@ proc weakrefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.sons.insert(des, 0)
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace: discard
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
var actions = newNodeI(nkStmtList, c.info)
Expand All @@ -771,6 +799,7 @@ proc ownedRefOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.add genIf(c, x, actions)
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace: discard
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
if c.kind == attachedDeepCopy:
Expand Down Expand Up @@ -805,6 +834,7 @@ proc closureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.sons.insert(des, 0)
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace: discard
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
let xx = genBuiltin(c, mAccessEnv, "accessEnv", x)
Expand All @@ -820,6 +850,7 @@ proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.add genIf(c, xx, actions)
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace: discard
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)

proc fillBody(c: var TLiftCtx; t: PType; body, x, y: PNode) =
case t.kind
Expand Down Expand Up @@ -935,7 +966,7 @@ proc symPrototype(g: ModuleGraph; typ: PType; owner: PSym; kind: TTypeAttachedOp

result.typ = newProcType(info, nextTypeId(idgen), owner)
result.typ.addParam dest
if kind != attachedDestructor:
if kind notin {attachedDestructor, attachedWasMoved}:
result.typ.addParam src

if kind == attachedAsgn and g.config.selectedGC == gcOrc and
Expand Down Expand Up @@ -975,7 +1006,7 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp;

let dest = result.typ.n[1].sym
let d = newDeref(newSymNode(dest))
let src = if kind == attachedDestructor: newNodeIT(nkSym, info, getSysType(g, info, tyPointer))
let src = if kind in {attachedDestructor, attachedWasMoved}: newNodeIT(nkSym, info, getSysType(g, info, tyPointer))
else: newSymNode(result.typ.n[2].sym)

# register this operation already:
Expand Down Expand Up @@ -1103,15 +1134,15 @@ proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInf
# bug #15122: We need to produce all prototypes before entering the
# mind boggling recursion. Hacks like these imply we should rewrite
# this module.
var generics: array[attachedDestructor..attachedTrace, bool]
for k in attachedDestructor..lastAttached:
var generics: array[attachedWasMoved..attachedTrace, bool]
for k in attachedWasMoved..lastAttached:
generics[k] = getAttachedOp(g, canon, k) != nil
if not generics[k]:
setAttachedOp(g, idgen.module, canon, k,
symPrototype(g, canon, canon.owner, k, info, idgen))

# we generate the destructor first so that other operators can depend on it:
for k in attachedDestructor..lastAttached:
for k in attachedWasMoved..lastAttached:
if not generics[k]:
discard produceSym(g, c, canon, k, info, idgen)
else:
Expand Down
9 changes: 6 additions & 3 deletions compiler/optimizer.nim
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import

from trees import exprStructuralEquivalent

import std/strutils

const
nfMarkForDeletion = nfNone # faster than a lookup table

Expand Down Expand Up @@ -110,16 +112,17 @@ proc analyse(c: var Con; b: var BasicBlock; n: PNode) =
var reverse = false
if n[0].kind == nkSym:
let s = n[0].sym
if s.magic == mWasMoved:
let name = s.name.s.normalize
if s.magic == mWasMoved or name == "=wasmoved":
b.wasMovedLocs.add n
special = true
elif s.name.s == "=destroy":
elif name == "=destroy":
if c.inFinally > 0 and (b.hasReturn or b.hasBreak):
discard "cannot optimize away the destructor"
else:
c.wasMovedDestroyPair b, n
special = true
elif s.name.s == "=sink":
elif name == "=sink":
reverse = true

if not special:
Expand Down
5 changes: 4 additions & 1 deletion compiler/semstmts.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1779,7 +1779,7 @@ proc whereToBindTypeHook(c: PContext; t: PType): PType =
proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) =
let t = s.typ
var noError = false
let cond = if op == attachedDestructor:
let cond = if op in {attachedDestructor, attachedWasMoved}:
t.len == 2 and t[0] == nil and t[1].kind == tyVar
elif op == attachedTrace:
t.len == 3 and t[0] == nil and t[1].kind == tyVar and t[2].kind == tyPointer
Expand Down Expand Up @@ -1894,6 +1894,9 @@ proc semOverride(c: PContext, s: PSym, n: PNode) =
of "=trace":
if s.magic != mTrace:
bindTypeHook(c, s, n, attachedTrace)
of "=wasmoved":
if s.magic != mWasMoved:
bindTypeHook(c, s, n, attachedWasMoved)
else:
if sfOverriden in s.flags:
localError(c.config, n.info, errGenerated,
Expand Down
2 changes: 1 addition & 1 deletion doc/manual.md
Original file line number Diff line number Diff line change
Expand Up @@ -4122,7 +4122,7 @@ the operator is in scope (including if it is private).
```

Type bound operators are:
`=destroy`, `=copy`, `=sink`, `=trace`, `=deepcopy`.
`=destroy`, `=copy`, `=sink`, `=trace`, `=deepcopy`, `=wasMoved`.

These operations can be *overridden* instead of *overloaded*. This means that
the implementation is automatically lifted to structured types. For instance,
Expand Down
19 changes: 10 additions & 9 deletions tests/arc/topt_no_cursor.nim
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
discard """
nimoutFull: true
cmd: '''nim c -r --warnings:off --hints:off --gc:arc --expandArc:newTarget --expandArc:delete --expandArc:p1 --expandArc:tt --hint:Performance:off --assertions:off --expandArc:extractConfig --expandArc:mergeShadowScope --expandArc:check $file'''
nimout: '''--expandArc: newTarget
nimout: '''
--expandArc: newTarget
var
splat
Expand All @@ -11,9 +12,9 @@ splat = splitDrive do:
let blitTmp = path
blitTmp
:tmp = splat.drive
wasMoved(splat.drive)
`=wasMoved`(splat.drive)
:tmp_1 = splat.path_1
wasMoved(splat.path_1)
`=wasMoved`(splat.path_1)
result = (
let blitTmp_1 = :tmp
blitTmp_1,
Expand Down Expand Up @@ -60,10 +61,10 @@ var
try:
it_cursor = x
a = (
wasMoved(:tmpD)
`=wasMoved`(:tmpD)
`=copy`(:tmpD, it_cursor.key)
:tmpD,
wasMoved(:tmpD_1)
`=wasMoved`(:tmpD_1)
`=copy`(:tmpD_1, it_cursor.val)
:tmpD_1)
echo [
Expand Down Expand Up @@ -112,7 +113,7 @@ block :tmp:
var :tmpD
sym = shadowScope.symbols[i]
addInterfaceDecl(c):
wasMoved(:tmpD)
`=wasMoved`(:tmpD)
`=copy_1`(:tmpD, sym)
:tmpD
inc(i, 1)
Expand All @@ -125,18 +126,18 @@ this.isValid = fileExists(this.value)
if dirExists(this.value):
var :tmpD
par = (dir:
wasMoved(:tmpD)
`=wasMoved`(:tmpD)
`=copy`(:tmpD, this.value)
:tmpD, front: "") else:
var
:tmpD_1
:tmpD_2
:tmpD_3
par = (dir_1: parentDir(this.value), front_1:
wasMoved(:tmpD_1)
`=wasMoved`(:tmpD_1)
`=copy`(:tmpD_1,
:tmpD_3 = splitDrive do:
wasMoved(:tmpD_2)
`=wasMoved`(:tmpD_2)
`=copy`(:tmpD_2, this.value)
:tmpD_2
:tmpD_3.path)
Expand Down
8 changes: 5 additions & 3 deletions tests/arc/topt_refcursors.nim
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
discard """
output: ''''''
cmd: '''nim c --gc:arc --expandArc:traverse --hint:Performance:off $file'''
nimout: '''--expandArc: traverse
nimout: '''
--expandArc: traverse
var
it_cursor
Expand All @@ -22,12 +23,13 @@ try:
`=copy`(ri_1, jt.ri)
echo [jt.s]
`=sink`(jt, ri_1)
wasMoved(ri_1)
`=wasMoved`(ri_1)
finally:
`=destroy`(ri_1)
finally:
`=destroy`(jt)
-- end of expandArc ------------------------'''
-- end of expandArc ------------------------
'''
"""

type
Expand Down
Loading

0 comments on commit a137e50

Please sign in to comment.