From cd1fa9a359ed55f6eaf8721b2d96ee2d72e23ee8 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 00:11:16 +0000 Subject: [PATCH 01/11] fixes #12989 --- compiler/lowerings.nim | 1 - tests/destructor/t12037.nim | 9 +++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/compiler/lowerings.nim b/compiler/lowerings.nim index ac29d600bf860..cf8445820580b 100644 --- a/compiler/lowerings.nim +++ b/compiler/lowerings.nim @@ -60,7 +60,6 @@ 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) diff --git a/tests/destructor/t12037.nim b/tests/destructor/t12037.nim index 57ebae9e4e5b7..1a7d536cc68f9 100644 --- a/tests/destructor/t12037.nim +++ b/tests/destructor/t12037.nim @@ -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)) From 1c21c79a9a03133835fcecf8b4387a5350404a29 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 11:35:30 +0000 Subject: [PATCH 02/11] optimization --- compiler/injectdestructors.nim | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 0acf7fac258f6..3882ef6970b95 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -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) & ">" @@ -542,7 +542,7 @@ 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) @@ -612,10 +612,13 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = case ri.kind of nkCallKinds: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + if isUnpackedTuple(dest): + result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) + else: + result = genSink(c, dest, ri) + result.add p(ri, c, consumed) of nkBracketExpr: - if ri[0].kind == nkSym and isUnpackedTuple(ri[0].sym): + if isUnpackedTuple(ri[0]): # unpacking of tuple: move out the elements result = genSink(c, dest, ri) result.add p(ri, c, consumed) From e918fa6ffc4843ddb311e94c23991fd6e0949262 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 12:10:27 +0000 Subject: [PATCH 03/11] optimization 2 --- compiler/injectdestructors.nim | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 3882ef6970b95..d9a17cf7a2bfd 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -136,7 +136,7 @@ proc initialized(code: ControlFlowGraph; pc: int, template isUnpackedTuple(n: PNode): bool = ## we move out all elements of unpacked tuples, ## hence unpacked tuples themselves don't need to be destroyed - n.kind == nkSym and n.sym.kind == skTemp and n.sym.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) & ">" @@ -619,9 +619,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = result.add p(ri, c, consumed) of nkBracketExpr: if isUnpackedTuple(ri[0]): - # unpacking of tuple: move out the elements - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + # 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) From 5f0c69e2e6561232662cc1c3c12821f3348e964a Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 12:13:04 +0000 Subject: [PATCH 04/11] fix test --- tests/destructor/tmisc_destructors.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/destructor/tmisc_destructors.nim b/tests/destructor/tmisc_destructors.nim index fdcea074b2041..7fc8a61c26b77 100644 --- a/tests/destructor/tmisc_destructors.nim +++ b/tests/destructor/tmisc_destructors.nim @@ -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 == 6 # bug #11510 proc main = From 501829732a8e44deef2d815c303859efbe452cb5 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 12:45:49 +0000 Subject: [PATCH 05/11] remove unwanted changes --- compiler/injectdestructors.nim | 7 ++----- tests/destructor/t12037.nim | 1 + 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index d9a17cf7a2bfd..f66986777f3d4 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -612,11 +612,8 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = case ri.kind of nkCallKinds: - if isUnpackedTuple(dest): - result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) - else: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + result = genSink(c, dest, ri) + result.add p(ri, c, consumed) of nkBracketExpr: if isUnpackedTuple(ri[0]): # unpacking of tuple: take over elements diff --git a/tests/destructor/t12037.nim b/tests/destructor/t12037.nim index 1a7d536cc68f9..4b17fe6d06d84 100644 --- a/tests/destructor/t12037.nim +++ b/tests/destructor/t12037.nim @@ -32,3 +32,4 @@ proc bug(start: (seq[int], int)) = let input = @[0] bug((input, 0)) +doASsert(input.len == 1) From c73fed7318b2f8f47273df921e418d7f5a69f15b Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 12:50:33 +0000 Subject: [PATCH 06/11] Revert "remove unwanted changes" This reverts commit 501829732a8e44deef2d815c303859efbe452cb5. --- compiler/injectdestructors.nim | 7 +++++-- tests/destructor/t12037.nim | 1 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index f66986777f3d4..d9a17cf7a2bfd 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -612,8 +612,11 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = case ri.kind of nkCallKinds: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + if isUnpackedTuple(dest): + result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) + else: + result = genSink(c, dest, ri) + result.add p(ri, c, consumed) of nkBracketExpr: if isUnpackedTuple(ri[0]): # unpacking of tuple: take over elements diff --git a/tests/destructor/t12037.nim b/tests/destructor/t12037.nim index 4b17fe6d06d84..1a7d536cc68f9 100644 --- a/tests/destructor/t12037.nim +++ b/tests/destructor/t12037.nim @@ -32,4 +32,3 @@ proc bug(start: (seq[int], int)) = let input = @[0] bug((input, 0)) -doASsert(input.len == 1) From 008022b4a6aea089c31fd65220c0e914229f7d48 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 19:25:07 +0000 Subject: [PATCH 07/11] MemMove optimization in inject destructors --- compiler/injectdestructors.nim | 29 +++++++++++++++----------- tests/destructor/tmisc_destructors.nim | 2 +- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index d9a17cf7a2bfd..d065c55a4b3b5 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -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 mem + 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) @@ -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] == ':' @@ -547,7 +552,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = 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) @@ -609,13 +614,13 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = for i in 0.. 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: @@ -663,7 +668,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: @@ -672,7 +677,7 @@ 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) @@ -680,7 +685,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = 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: diff --git a/tests/destructor/tmisc_destructors.nim b/tests/destructor/tmisc_destructors.nim index 7fc8a61c26b77..7d6995e4e17e3 100644 --- a/tests/destructor/tmisc_destructors.nim +++ b/tests/destructor/tmisc_destructors.nim @@ -28,7 +28,7 @@ proc test(): auto = var (a, b, _) = test() doAssert assign_counter == 0 -doAssert sink_counter == 6 +doAssert sink_counter == 3 # bug #11510 proc main = From 41f1c730c56e05a05cfb3752509674d5b5026dfb Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 19:26:21 +0000 Subject: [PATCH 08/11] fix comment --- compiler/injectdestructors.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index d065c55a4b3b5..9403335ae18ed 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -213,7 +213,7 @@ proc genSink(c: Con; dest, ri: PNode): PNode = result = newTree(nkFastAsgn, dest) proc genSinkOrMemMove(c: Con; dest, ri: PNode, isFirstWrite: bool): PNode = - # optimize sink call into a bitwise mem + # optimize sink call into a bitwise memcopy if isFirstWrite: newTree(nkFastAsgn, dest) else: genSink(c, dest, ri) From 1c6ebcf1104376fe31513fefe434e5baf2ba6ccc Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko Date: Tue, 31 Dec 2019 19:52:35 +0000 Subject: [PATCH 09/11] generalize tuple optimization --- compiler/injectdestructors.nim | 7 ++----- compiler/liftdestructors.nim | 7 ------- compiler/lowerings.nim | 10 ++++++++-- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 9403335ae18ed..9d55ad3291ca8 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -617,11 +617,8 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite = false): PNode = case ri.kind of nkCallKinds: - if isUnpackedTuple(dest): - result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) - else: - result = genSinkOrMemMove(c, dest, ri, isFirstWrite) - result.add p(ri, c, consumed) + result = genSinkOrMemMove(c, dest, ri, isFirstWrite) + result.add p(ri, c, consumed) of nkBracketExpr: if isUnpackedTuple(ri[0]): # unpacking of tuple: take over elements diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 932b24bb097b2..daf91954b05be 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -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) diff --git a/compiler/lowerings.nim b/compiler/lowerings.nim index cf8445820580b..d069bba84171b 100644 --- a/compiler/lowerings.nim +++ b/compiler/lowerings.nim @@ -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 @@ -63,10 +70,9 @@ proc lowerTupleUnpacking*(g: ModuleGraph; n: PNode; owner: PSym): PNode = var v = newNodeI(nkVarSection, value.info) let tempAsNode = newSymNode(temp) - v.addVar(tempAsNode) + v.addVar(tempAsNode, value) result.add(v) - result.add newAsgnStmt(tempAsNode, value) for i in 0.. Date: Tue, 31 Dec 2019 20:42:37 +0000 Subject: [PATCH 10/11] more optimization --- compiler/injectdestructors.nim | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 9d55ad3291ca8..b2cf0049c4ce5 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -289,7 +289,7 @@ type sinkArg proc p(n: PNode; c: var Con; mode: ProcessMode): PNode -proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite = false): PNode +proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite: bool): PNode proc isClosureEnv(n: PNode): bool = n.kind == nkSym and n.sym.name.s[0] == ':' @@ -571,7 +571,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = else: if n[0].kind in {nkDotExpr, nkCheckedFieldExpr}: cycleCheck(n, c) - result = moveOrCopy(n[0], n[1], c) + result = moveOrCopy(n[0], n[1], c, isFirstWrite = false) else: result = copyNode(n) result.add copyTree(n[0]) @@ -614,7 +614,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = for i in 0.. Date: Thu, 2 Jan 2020 19:24:32 +0000 Subject: [PATCH 11/11] bug fix --- compiler/injectdestructors.nim | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index b2cf0049c4ce5..fad1eb03b9ff8 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -617,8 +617,11 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = proc moveOrCopy(dest, ri: PNode; c: var Con, isFirstWrite: bool): PNode = case ri.kind of nkCallKinds: - result = genSinkOrMemMove(c, dest, ri, isFirstWrite) - result.add p(ri, c, consumed) + if isUnpackedTuple(dest): + result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) + else: + result = genSinkOrMemMove(c, dest, ri, isFirstWrite) + result.add p(ri, c, consumed) of nkBracketExpr: if isUnpackedTuple(ri[0]): # unpacking of tuple: take over elements