From 659a58ef66c98c1f90d4386cf8a60a59e4492ff1 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Fri, 13 Oct 2023 01:29:11 +0200 Subject: [PATCH] injectdestructors: remove obsolete processing (#956) ## Summary Remove the moving of `def`s from the destructor injection pass. It is obsolete now that unscoped `def`s are properly handled during the MIR -> `CgNode` translation. ## Details Moving the `def`s was necessary back when their semantics weren't as fleshed out, but with the behaviour of a `def` statements now being clearer, and `cgirgen` properly translating them, this step is unnecessary. Not moving the `def` during destructor injection also improves the final generated code, as the regions previously used for turning a `def` statement into an assignment often assigned the input value to an, in this case, unnecessary temporary first. --- compiler/sem/injectdestructors.nim | 36 ++-------------------- tests/arc/topt_cursor.nim | 9 +++--- tests/arc/topt_no_cursor.nim | 21 ++++++------- tests/lang_objects/destructor/tv2_cast.nim | 34 +++++++++----------- 4 files changed, 30 insertions(+), 70 deletions(-) diff --git a/compiler/sem/injectdestructors.nim b/compiler/sem/injectdestructors.nim index 5f0f84a3edf..e652499ba96 100644 --- a/compiler/sem/injectdestructors.nim +++ b/compiler/sem/injectdestructors.nim @@ -1006,37 +1006,6 @@ proc injectDestructors(tree: MirTree, graph: ModuleGraph, result = ord(x.pos) - ord(y.pos) ) - # second pass: if at least one entity in a scope needs its destructor call - # placed in a ``finally`` clause, all others in the same scope do too, as the - # order-of-destruction would be violated otherwise - for pos, scope in entries.items: - if scope in needsFinally: - # if the destroy call has to be placed inside a ``finally`` clause, we - # first need to move the 'def' from its current position to the start of - # the scope, as it'd be otherwise located inside the ``try``'s body - # (which would render the entity unavailable inside the ``finally`` - # clause) - assert tree[pos].kind == mnkDef - c.seek(pos) - if hasInput(tree, Operation pos): - # replace the 'def' with an initializing assignment if it has an - # input: - c.replaceMulti(buf): - buf.subTree MirNode(kind: mnkRegion): - argBlock(buf): - let e = getDefEntity(tree, pos) # the entity (e.g. local) - chain(buf): emit(tree[e]) => name() - chain(buf): opParam(0, tree[e].typ) => arg() - buf.add MirNode(kind: mnkInit) - - else: - c.remove() - - # insert the 'def' at the start of the scope: - c.seek(scope + 1) - c.insert(NodeInstance pos, buf): - buf.add toOpenArray(tree, pos.int, pos.int+2) - iterator scopeItems(e: seq[DestroyEntry]): Slice[int] {.inline.} = ## Partitions `e` using the `scope` field and yields the slice of each ## partition @@ -1052,7 +1021,7 @@ proc injectDestructors(tree: MirTree, graph: ModuleGraph, scopePos = e[i].scope start = i - # third pass: inject the destructors and place them inside a ``finally`` + # second pass: inject the destructors and place them inside a ``finally`` # clause if necessary for s in scopeItems(entries): let @@ -1062,8 +1031,7 @@ proc injectDestructors(tree: MirTree, graph: ModuleGraph, ## the node to inherit the origin information from if useFinally: - # at the start of the scope (after the 'def's previously moved there), - # insert the start nodes of a 'try' and a 'stmtList': + # start a 'finally' at the beginning of the scope: c.seek(scopeStart + 1) c.insert(source, buf): buf.add MirNode(kind: mnkTry, len: 1) diff --git a/tests/arc/topt_cursor.nim b/tests/arc/topt_cursor.nim index 1395c242f7b..f324875b247 100644 --- a/tests/arc/topt_cursor.nim +++ b/tests/arc/topt_cursor.nim @@ -4,7 +4,7 @@ discard """ nimout: '''--expandArc: main var x_cursor -var :aux_2 +var :aux_3 try: x_cursor = ("hi", 5) block :label_0: @@ -13,11 +13,10 @@ try: break :label_0 x_cursor = [type node](("string here", 80)) echo([ - var :aux_4 = $(x_cursor) - :aux_2 = :aux_4 - :aux_2]) + :aux_3 = $(x_cursor) + :aux_3]) finally: - =destroy(:aux_2) + =destroy(:aux_3) -- end of expandArc ------------------------ --expandArc: sio diff --git a/tests/arc/topt_no_cursor.nim b/tests/arc/topt_no_cursor.nim index dcbbe898c0b..5b387a5b9f8 100644 --- a/tests/arc/topt_no_cursor.nim +++ b/tests/arc/topt_no_cursor.nim @@ -60,25 +60,24 @@ result.value = move(lvalue) --expandArc: tt var it_cursor +var a +var :aux_4 var :aux_5 var :aux_6 -var a -var :aux_3 try: it_cursor = x a = ( + :aux_4 = default() + =copy(:aux_4, it_cursor.key) + :aux_4, :aux_5 = default() - =copy(:aux_5, it_cursor.key) - :aux_5, - :aux_6 = default() - =copy(:aux_6, it_cursor.val) - :aux_6) + =copy(:aux_5, it_cursor.val) + :aux_5) echo([ - var :aux_7 = $(a) - :aux_3 = :aux_7 - :aux_3]) + :aux_6 = $(a) + :aux_6]) finally: - =destroy(:aux_3) + =destroy(:aux_6) =destroy_1(a) -- end of expandArc ------------------------ --expandArc: extractConfig diff --git a/tests/lang_objects/destructor/tv2_cast.nim b/tests/lang_objects/destructor/tv2_cast.nim index ba6b8709027..746e336657e 100644 --- a/tests/lang_objects/destructor/tv2_cast.nim +++ b/tests/lang_objects/destructor/tv2_cast.nim @@ -9,13 +9,11 @@ var data var :aux_2 var :aux_3 try: - var :aux_5 = encode( - var :aux_4 = newString(100) - :aux_2 = :aux_4 + :aux_3 = encode( + :aux_2 = newString(100) cast[seq[byte]](:aux_2)) - :aux_3 = :aux_5 - var :aux_6 = cast[string](:aux_3) - =copy(data, :aux_6) + var :aux_4 = cast[string](:aux_3) + =copy(data, :aux_4) finally: =destroy(:aux_3) =destroy_1(:aux_2) @@ -27,10 +25,9 @@ var data var :aux_3 try: s = newString(100) - var :aux_4 = encode(toOpenArrayByte(s, 0, -(len(s), 1))) - :aux_3 = :aux_4 - var :aux_5 = cast[string](:aux_3) - =copy(data, :aux_5) + :aux_3 = encode(toOpenArrayByte(s, 0, -(len(s), 1))) + var :aux_4 = cast[string](:aux_3) + =copy(data, :aux_4) finally: =destroy(:aux_3) =destroy_1(data) @@ -42,10 +39,9 @@ var data var :aux_3 try: s = newSeq(100) - var :aux_4 = encode(s) - :aux_3 = :aux_4 - var :aux_5 = cast[string](:aux_3) - =copy(data, :aux_5) + :aux_3 = encode(s) + var :aux_4 = cast[string](:aux_3) + =copy(data, :aux_4) finally: =destroy(:aux_3) =destroy_1(data) @@ -56,13 +52,11 @@ var data var :aux_2 var :aux_3 try: - var :aux_5 = encode( - var :aux_4 = newSeq(100) - :aux_2 = :aux_4 + :aux_3 = encode( + :aux_2 = newSeq(100) :aux_2) - :aux_3 = :aux_5 - var :aux_6 = cast[string](:aux_3) - =copy(data, :aux_6) + var :aux_4 = cast[string](:aux_3) + =copy(data, :aux_4) finally: =destroy(:aux_3) =destroy(:aux_2)