Skip to content

Sink to MemMove optimization in injectdestructors #13002

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

Merged
merged 12 commits into from
Jan 2, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions compiler/injectdestructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,10 @@ proc initialized(code: ControlFlowGraph; pc: int,
inc pc
return pc

template isUnpackedTuple(s: PSym): bool =
template isUnpackedTuple(n: PNode): bool =
## we move out all elements of unpacked tuples,
## hence unpacked tuples themselves don't need to be destroyed
s.kind == skTemp and s.typ.kind == tyTuple
(n.kind == nkSym and n.sym.kind == skTemp and n.sym.typ.kind == tyTuple)

proc checkForErrorPragma(c: Con; t: PType; ri: PNode; opname: string) =
var m = "'" & opname & "' is not available for type <" & typeToString(t) & ">"
Expand Down Expand Up @@ -212,6 +212,11 @@ proc genSink(c: Con; dest, ri: PNode): PNode =
# we generate a fast assignment in this case:
result = newTree(nkFastAsgn, dest)

proc genSinkOrMemMove(c: Con; dest, ri: PNode, isFirstWrite: bool): PNode =
# optimize sink call into a bitwise memcopy
if isFirstWrite: newTree(nkFastAsgn, dest)
else: genSink(c, dest, ri)

proc genCopyNoCheck(c: Con; dest, ri: PNode): PNode =
let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink})
result = genOp(c, t, attachedAsgn, dest, ri)
Expand Down Expand Up @@ -284,7 +289,7 @@ type
sinkArg

proc p(n: PNode; c: var Con; mode: ProcessMode): PNode
proc moveOrCopy(dest, ri: PNode; c: var Con): PNode
proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite = false): PNode

proc isClosureEnv(n: PNode): bool = n.kind == nkSym and n.sym.name.s[0] == ':'

Expand Down Expand Up @@ -542,12 +547,12 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode =
# move the variable declaration to the top of the frame:
c.addTopVar v
# make sure it's destroyed at the end of the proc:
if not isUnpackedTuple(it[0].sym):
if not isUnpackedTuple(v):
c.destroys.add genDestroy(c, v)
if ri.kind == nkEmpty and c.inLoop > 0:
ri = genDefaultCall(v.typ, c, v.info)
if ri.kind != nkEmpty:
let r = moveOrCopy(v, ri, c)
let r = moveOrCopy(v, ri, c, isFirstWrite = (c.inLoop == 0))
result.add r
else: # keep the var but transform 'ri':
var v = copyNode(n)
Expand Down Expand Up @@ -609,19 +614,18 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode =
for i in 0..<n.len:
result[i] = p(n[i], c, mode)

proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite = false): PNode =
case ri.kind
of nkCallKinds:
result = genSink(c, dest, ri)
result = genSinkOrMemMove(c, dest, ri, isFirstWrite)
result.add p(ri, c, consumed)
of nkBracketExpr:
if ri[0].kind == nkSym and isUnpackedTuple(ri[0].sym):
# unpacking of tuple: move out the elements
result = genSink(c, dest, ri)
result.add p(ri, c, consumed)
if isUnpackedTuple(ri[0]):
# unpacking of tuple: take over elements
result = newTree(nkFastAsgn, dest, p(ri, c, consumed))
elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c):
# Rule 3: `=sink`(x, z); wasMoved(z)
var snk = genSink(c, dest, ri)
var snk = genSinkOrMemMove(c, dest, ri, isFirstWrite)
snk.add ri
result = newTree(nkStmtList, snk, genWasMoved(ri, c))
else:
Expand All @@ -632,22 +636,22 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
if ri.len > 0 and isDangerousSeq(ri.typ):
result = genCopy(c, dest, ri)
else:
result = genSink(c, dest, ri)
result = genSinkOrMemMove(c, dest, ri, isFirstWrite)
result.add p(ri, c, consumed)
of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
result = genSink(c, dest, ri)
result = genSinkOrMemMove(c, dest, ri, isFirstWrite)
result.add p(ri, c, consumed)
of nkSym:
if isSinkParam(ri.sym):
# Rule 3: `=sink`(x, z); wasMoved(z)
sinkParamIsLastReadCheck(c, ri)
var snk = genSink(c, dest, ri)
var snk = genSinkOrMemMove(c, dest, ri, isFirstWrite)
snk.add ri
result = newTree(nkStmtList, snk, genWasMoved(ri, c))
elif ri.sym.kind != skParam and ri.sym.owner == c.owner and
isLastRead(ri, c) and canBeMoved(c, dest.typ):
# Rule 3: `=sink`(x, z); wasMoved(z)
var snk = genSink(c, dest, ri)
var snk = genSinkOrMemMove(c, dest, ri, isFirstWrite)
snk.add ri
result = newTree(nkStmtList, snk, genWasMoved(ri, c))
else:
Expand All @@ -661,7 +665,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
copyRi[1] = result[^1]
result[^1] = copyRi
else:
result = genSink(c, dest, ri)
result = genSinkOrMemMove(c, dest, ri, isFirstWrite)
result.add p(ri, c, sinkArg)
of nkObjDownConv, nkObjUpConv:
when false:
Expand All @@ -670,15 +674,15 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
copyRi[0] = result[^1]
result[^1] = copyRi
else:
result = genSink(c, dest, ri)
result = genSinkOrMemMove(c, dest, ri, isFirstWrite)
result.add p(ri, c, sinkArg)
of nkStmtListExpr, nkBlockExpr, nkIfExpr, nkCaseStmt:
handleNested(ri): moveOrCopy(dest, node, c)
else:
if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and
canBeMoved(c, dest.typ):
# Rule 3: `=sink`(x, z); wasMoved(z)
var snk = genSink(c, dest, ri)
var snk = genSinkOrMemMove(c, dest, ri, isFirstWrite)
snk.add ri
result = newTree(nkStmtList, snk, genWasMoved(ri, c))
else:
Expand Down
7 changes: 0 additions & 7 deletions compiler/liftdestructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,6 @@ proc considerUserDefinedOp(c: var TLiftCtx; t: PType; body, x, y: PNode): bool =
body.add newDeepCopyCall(op, x, y)
result = true

proc addVar(father, v, value: PNode) =
var vpart = newNodeI(nkIdentDefs, v.info, 3)
vpart[0] = v
vpart[1] = newNodeI(nkEmpty, v.info)
vpart[2] = value
father.add vpart

proc declareCounter(c: var TLiftCtx; body: PNode; first: BiggestInt): PNode =
var temp = newSym(skTemp, getIdent(c.g.cache, lowerings.genPrefix), c.fn, c.info)
temp.typ = getSysType(c.g, body.info, tyInt)
Expand Down
11 changes: 8 additions & 3 deletions compiler/lowerings.nim
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ proc addVar*(father, v: PNode) =
vpart[2] = vpart[1]
father.add vpart

proc addVar*(father, v, value: PNode) =
var vpart = newNodeI(nkIdentDefs, v.info, 3)
vpart[0] = v
vpart[1] = newNodeI(nkEmpty, v.info)
vpart[2] = value
father.add vpart

proc newAsgnStmt*(le, ri: PNode): PNode =
result = newNodeI(nkAsgn, le.info, 2)
result[0] = le
Expand All @@ -60,14 +67,12 @@ proc lowerTupleUnpacking*(g: ModuleGraph; n: PNode; owner: PSym): PNode =
var temp = newSym(skTemp, getIdent(g.cache, genPrefix), owner, value.info, g.config.options)
temp.typ = skipTypes(value.typ, abstractInst)
incl(temp.flags, sfFromGeneric)
incl(temp.flags, sfCursor)

var v = newNodeI(nkVarSection, value.info)
let tempAsNode = newSymNode(temp)
v.addVar(tempAsNode)
v.addVar(tempAsNode, value)
Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed lowerTupleUnpacking because now pattern var x = value is optimized while var x; x = ... is not.

result.add(v)

result.add newAsgnStmt(tempAsNode, value)
for i in 0..<n.len-2:
if n[i].kind == nkSym: v.addVar(n[i])
result.add newAsgnStmt(n[i], newTupleAccess(g, tempAsNode, i))
Expand Down
9 changes: 9 additions & 0 deletions tests/destructor/t12037.nim
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,12 @@ test()
import tables
var t = initTable[string, seq[ptr int]]()
discard t.hasKeyOrPut("f1", @[])


#############################################
### bug #12989
proc bug(start: (seq[int], int)) =
let (s, i) = start

let input = @[0]
bug((input, 0))
2 changes: 1 addition & 1 deletion tests/destructor/tmisc_destructors.nim
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ proc test(): auto =
var (a, b, _) = test()

doAssert assign_counter == 0
doAssert sink_counter == 9 # XXX this is still silly and needs to be investigated
doAssert sink_counter == 3

# bug #11510
proc main =
Expand Down