Skip to content

Commit

Permalink
internal: remove the double-meaning of mMove (#968)
Browse files Browse the repository at this point in the history
## Summary

Don't use a special `move` magic form for the `seq`/`string` sink
implementation, but instead emit the required code directly. This moves
decision making out the code generators, fixes a MIR-level issue, and
is a preparation for MIR improvements.

## Details

For the `=sink` implementation of `seq`s and `string`s,
`liftdestructors` emitted a special `mMove` call that the C code
generator then expanded into the C equivalent of:

```nim
if not samePayload(a, b):
  =destroy(a)
a = b
```

The problem is with how the generated `move` call looked:
`move(a, b, =destroy(a))`. The `CgNode` IR for the call has to have the
same shape, but this required special-casing in `mirgen` and the
resulting MIR code also doesn't properly represent the actual
behaviour (the `=destroy(a)` call is not unconditionally evaluated),
which could interfere with MIR analysis.

In order to correctly represent the behaviour at the MIR level, the
`liftdestructors` lifting emits the above code directly, without
overloading the `mMove` magic. Since how the payload comparison works
is currently backend dependent, the new internal-only `mSamePayload`
magic is introduced, which the C generator then lowers into a
comparison of the payload pointers.
  • Loading branch information
zerbina authored Oct 17, 2023
1 parent d0ff868 commit 7db09b1
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 56 deletions.
2 changes: 2 additions & 0 deletions compiler/ast/ast_types.nim
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,8 @@ type
mChckRange
## chckRange(v, lower, upper); conversion + range check -- returns
## either the type-converted value or raises a defect
mSamePayload
## returns whether both seq/string operands share the same payload

# things that we can evaluate safely at compile time, even if not asked for it:
const
Expand Down
17 changes: 8 additions & 9 deletions compiler/backend/ccgexprs.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1689,15 +1689,8 @@ proc genWasMoved(p: BProc; n: CgNode) =

proc genMove(p: BProc; n: CgNode; d: var TLoc) =
var a: TLoc
initLocExpr(p, n[1].skipAddr, a)
if n.len == 4:
# generated by liftdestructors:
var src: TLoc
initLocExpr(p, n[2], src)
linefmt(p, cpsStmts, "if ($1.p != $2.p) {", [rdLoc(a), rdLoc(src)])
genStmts(p, n[3])
linefmt(p, cpsStmts, "}$n$1.len = $2.len; $1.p = $2.p;$n", [rdLoc(a), rdLoc(src)])
else:
initLocExpr(p, n[1], a)
if true:
if d.k == locNone: getTemp(p, n.typ, d)
genAssignment(p, d, a)
resetLoc(p, a)
Expand Down Expand Up @@ -1867,6 +1860,12 @@ proc genMagicExpr(p: BProc, e: CgNode, d: var TLoc, op: TMagic) =
linefmt(p, cpsStmts, "$1 = ($2)($3);$n", [a.r, typ, rdLoc(b)])
of mChckRange:
genRangeChck(p, e, d)
of mSamePayload:
var a, b: TLoc
initLocExpr(p, e[1], a)
initLocExpr(p, e[2], b)
# compare the payloads:
putIntoDest(p, d, e, "($1.p == $2.p)" % [rdLoc(a), rdLoc(b)])
else:
when defined(debugMagics):
echo p.prc.name.s, " ", p.prc.id, " ", p.prc.flags, " ", p.prc.ast[genericParamsPos].kind
Expand Down
35 changes: 2 additions & 33 deletions compiler/mir/mirgen.nim
Original file line number Diff line number Diff line change
Expand Up @@ -796,38 +796,6 @@ proc genMagic(c: var TCtx, n: PNode; m: TMagic): EValue =
argBlock(c.stmts):
chain(c): argExpr(c, n[1]) => tag(ekMutate) => name()
magicCall(c, m, typeOrVoid(c, n.typ))
of mMove:
# there exist two different types of ``mMove`` magic calls:
# 1. normal move calls: ``move(x)``
# 2. special move calls inserted by ``liftdestructors``, used for ``seq``s
# and ``string``s: ``move(dst, src, stmt)``
case n.len
of 2:
# the first version
genCall(c, n)
of 4:
# HACK: the ``stmt`` statement is not always evaluated, so treating it as
# a ``void`` argument is wrong. We also can't lower the call here
# already, since we don't have access to the ``seq``s
# implementation details and the lowering is also only required for
# the C target.
# Treating the statement as a ``void`` argument only works because,
# at the time of writing this comment, there are no MIR passes that
# inspect code in which a special ``move`` occurs.
# A more proper solution is to add a new magic (something like
# ``mMoveSeq``) that then gets lowered into the expected
# comparision + destructor call by a MIR pass that is only enabled
# for the C target

argBlock(c.stmts):
chain(c): argExpr(c, n[1]) => tag(ekReassign) => name()
chain(c): argExpr(c, n[2]) => arg()

gen(c, n[3])
chain(c): genEmpty(c, n[3]) => arg()
magicCall(c, m, typeOrVoid(c, n.typ))
else:
unreachable()
of mNewSeq:
# XXX: the first parameter is actually an ``out`` parameter -- the
# ``ekReassign`` effect could be used
Expand All @@ -846,7 +814,8 @@ proc genMagic(c: var TCtx, n: PNode; m: TMagic): EValue =
magicCall(c, m, typeOrVoid(c, n.typ))
else:
genCall(c, n)
of mLtI, mSubI, mLengthSeq, mLengthStr, mAccessEnv, mAccessTypeField:
of mNot, mLtI, mSubI, mLengthSeq, mLengthStr, mAccessEnv, mAccessTypeField,
mSamePayload:
if n[0].typ == nil:
# simple translation. None of the arguments need to be passed by lvalue
argBlock(c.stmts):
Expand Down
27 changes: 13 additions & 14 deletions compiler/sem/liftdestructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -515,11 +515,18 @@ proc fillSeqOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.add setLenSeqCall(c, t, x, y)
forallElements(c, t, body, x, y)
of attachedSink:
let moveCall = genBuiltin(c, mMove, "move", x)
moveCall.add y
doAssert t.destructor != nil
moveCall.add destructorCall(c, t.destructor, x)
body.add moveCall
# generate:
# if not samePayload(x, y):
# =destroy(x)
# x = y
let eq = genBuiltin(c, mSamePayload, "samePayload", x)
eq.add y
eq.typ = getSysType(c.g, x.info, tyBool)
let nt = genBuiltin(c, mNot, "not", eq)
nt.typ = eq.typ
body.add genIf(c, nt, destructorCall(c, t.destructor, x))
body.add newAsgnStmt(x, y)
of attachedDestructor:
# destroy all elements:
forallElements(c, t, body, x, y)
Expand Down Expand Up @@ -550,11 +557,7 @@ proc useSeqOrStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
body.add newHookCall(c, t.assignment, x, y)
of attachedSink:
# we always inline the move for better performance:
let moveCall = genBuiltin(c, mMove, "move", x)
moveCall.add y
doAssert t.destructor != nil
moveCall.add destructorCall(c, t.destructor, x)
body.add moveCall
fillSeqOp(c, t, body, x, y)
# alternatively we could do this:
when false:
doAssert t.asink != nil
Expand All @@ -574,11 +577,7 @@ proc fillStrOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
of attachedAsgn, attachedDeepCopy:
body.add callCodegenProc(c.g, "nimAsgnStrV2", c.info, genAddr(c, x), y)
of attachedSink:
let moveCall = genBuiltin(c, mMove, "move", x)
moveCall.add y
doAssert t.destructor != nil
moveCall.add destructorCall(c, t.destructor, x)
body.add moveCall
fillSeqOp(c, t, body, x, y)
of attachedDestructor:
body.add genBuiltin(c, mDestroy, "destroy", x)
of attachedTrace:
Expand Down

0 comments on commit 7db09b1

Please sign in to comment.