cgirgen: fix translation of defs (#954)

## Summary

Properly translate local definitions from the MIR to the `CgNode` IR.
While the previous implementation didn't produce the wrong results (so
far), it was based on incorrect assumptions.

## Details

* add `tbBody` and use it for translating the body of control-flow
  constructs (`mnkIf`, `mnkBlock`, etc.)
* replace the `inArgBlock` counter with the `inUnscoped` flag
* remove the `tbStmt` overload that's now only used in a single place
* instead of storing `cnkDef` nodes in the `defs` list, store the
  information required to produce the nodes; this is more efficient, as
  it means less work during clean-up (the contents of the `defs` seq
  don't require destruction)

### Translation of `def`s

A `def` for a local/global in the MIR means "locations starts to exist
and is accessible from all connected basic blocks within the same scope
(`mnkScope`) to which the only control-flow path also visits the def".
The `CgNode` IR doesn't have an explicit notion of "scope" as the MIR
does, and placing a `cnkDef` in some control-flow- related construct
(e.g., `cnkIf`) can produce the wrong code when the construct doesn't
open a new scope itself (i.e., its body is not wrapped in an
`mnkScope`).

Previously, translating a `mnkDef` to an assignment, with the `cnkDef`
placed at the start of the enclosing scope (this is also referred to as
lifting the def), was performed for all defs within an argument block.
However, argument blocks are wholly unrelated to the problem described
above, and the associated comment was also wrong: a `cnkStmtList`
doesn't open a scope.

Due to a lot of code being located in some argument block, this
incorrect implementation still produced the correct results so far, but
changes to how MIR code is generated or how some of the MIR passes
operate could break this at any time.

The correct thing to look for is whether a definition is inside a
control-flow constructs (`if`, `block`, etc.). If it is, and there's no
`mnkScope` in-between, the definition needs to be lifted into the
closest surrounding scope.

An alternative approach would be introducing a `cnkScope` node for the
`CgNode`  IR and then leaving the work to the code generators. The
reason
for not going that route is that keeping the lifting logic in `cgirgen`
for now makes it easier to change later on.

### Future work

* only locals that are used outside of their enclosing control-flow
  construct need to be lifted; an analysis pass could detect whether
  this is the case
* now that the semantics are clearer, the lifting of defs in
  the destructor injection pass can be removed

<!--
Pull Request(PR) Help

Before Merge Ensure:
* title reads like a short changelog line entry
* code includes tests and is documented
* leave the source better than before, but split out big reformats

See contributor
(guide)[https://nim-works.github.io/nimskull/contributing.html]
for details, especially if you're new to this project.

Tips that make PRs easier:
* for big/impactful changes, start with chat/discussions to refine
ideas
* refine the pull request message over time; don't have to nail it in
one go

compiler/backend/cgirgen.nim

@@ -114,19 +114,18 @@ type
 
     params: Values
 
-    # While in the MIR only a ``mnkScope`` opens a new scope, in ``CgPNode``-IR
-    # both ``cnkStmtList`` and ``cnkStmtListExpr`` do - the latter being used by
-    # the arg-block translation. A 'def'-like can appear inside an arg-block
-    # and the defined entity be used outside of it, which would thus result
-    # in the definition being placed in an ``cnkStmtListExpr``, producing
-    # semantically invalid code that later results in code-gen errors.
-    # To solve the problem, if a 'def'-like appears nested inside an arg-block,
-    # only an assignment (if necessary) is produced and the symbol node is
-    # added to the `def` list, which is then used to create a var section that
-    # is prepended to the statement list produced for the current enclosing
-    # ``mnkScope``
-    inArgBlock: int ## keeps track of the current arg-block nesting
-    defs: seq[CgNode]
+    # a 'def' in the MIR means that the the local starts to exists and that it
+    # is accessible in all connected basic blocks part of the enclosing
+    # ``mnkScope``. The ``CgNode`` IR doesn't use same notion of scope,
+    # so for now, all 'def's (without the initial values) within nested
+    # control-flow-related trees are moved to the start of the enclosing
+    # ``mnkScope``.
+    inUnscoped: bool
+      ## whether the currently proceesed statement/expression is part of an
+      ## unscoped control-flow context
+    defs: seq[tuple[local: LocalId, info: TLineInfo]]
+      ## the stack of locals for which the ``cnkDef`` needs to be inserted
+      ## later
 
   TreeWithSource = object
     ## Combines a ``MirTree`` with its associated ``SourceMap`` for
@@ -725,7 +724,8 @@ proc genObjConv(n: CgNode, a, b, t: PType): CgNode =
 # forward declarations:
 proc tbSeq(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor): Values
 
-proc tbStmt(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor): CgNode {.inline.}
+proc tbStmt(tree: TreeWithSource, cl: var TranslateCl, n: MirNode,
+            cr: var TreeCursor): CgNode
 proc tbList(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor): CgNode
 
 proc tbScope(tree: TreeWithSource, cl: var TranslateCl, n: MirNode, cr: var TreeCursor): CgNode
@@ -857,11 +857,10 @@ proc tbDef(tree: TreeWithSource, cl: var TranslateCl, prev: sink Values,
 
   case def.kind
   of cnkLocal:
-    if cl.inArgBlock > 0:
-      # if we're inside an arg-block, the var section is generated later and
-      # placed at an earlier position. We just produce an assignment to the
-      # entity here (if the def has an input)
-      cl.defs.add newStmt(cnkDef, info, [def, newEmpty()])
+    if cl.inUnscoped:
+      # add the local to the list of moved definitions and only emit an
+      # assignment
+      cl.defs.add (def.local, info)
       result =
         case prev.kind
         of vkNone:   newEmpty(info)
@@ -891,10 +890,20 @@ proc translateNode(n: PNode): CgNode =
     # cannot reach here
     unreachable(n.kind)
 
+proc tbBody(tree: TreeWithSource, cl: var TranslateCl,
+            cr: var TreeCursor): CgNode =
+  ## Generates the ``CgNode`` tree for the body of a construct that implies
+  ## some form of control-flow.
+  let prev = cl.inUnscoped
+  # assume the body is unscoped until stated otherwise
+  cl.inUnscoped = true
+  result = tbStmt(tree, cl, get(tree, cr), cr)
+  cl.inUnscoped = prev
+
 proc tbSingleStmt(tree: TreeWithSource, cl: var TranslateCl, n: MirNode,
                   cr: var TreeCursor): CgNode =
   template body(): CgNode =
-    tbStmt(tree, cl, cr)
+    tbBody(tree, cl, cr)
 
   let info = cr.info ## the source information of `n`
 
@@ -972,10 +981,6 @@ proc tbSingleStmt(tree: TreeWithSource, cl: var TranslateCl,
                   cr: var TreeCursor): CgNode {.inline.} =
   tbSingleStmt(tree, cl, get(tree, cr), cr)
 
-proc tbStmt(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor
-           ): CgNode {.inline.} =
-  tbStmt(tree, cl, get(tree, cr), cr)
-
 proc tbCaseStmt(tree: TreeWithSource, cl: var TranslateCl, n: MirNode,
                 prev: sink Values, cr: var TreeCursor): CgNode =
   result = newStmt(cnkCaseStmt, cr.info, [prev.single])
@@ -987,7 +992,7 @@ proc tbCaseStmt(tree: TreeWithSource, cl: var TranslateCl, n: MirNode,
       for x in 0..<br.len:
         result[^1].add translateLit(get(tree, cr).lit)
 
-    result[^1].add tbStmt(tree, cl, cr)
+    result[^1].add tbBody(tree, cl, cr)
     leave(tree, cr)
 
   leave(tree, cr)
@@ -1013,7 +1018,7 @@ proc tbOut(tree: TreeWithSource, cl: var TranslateCl, prev: sink Values,
     newStmt(cnkFastAsgn, cr.info, [prev[0], prev[1]])
   of mnkIf:
     assert prev.kind == vkSingle
-    let n = newStmt(cnkIfStmt, cr.info, [prev.single, tbStmt(tree, cl, cr)])
+    let n = newStmt(cnkIfStmt, cr.info, [prev.single, tbBody(tree, cl, cr)])
     leave(tree, cr)
 
     n
@@ -1043,8 +1048,6 @@ proc tbArgBlock(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor
   var stmts: seq[CgNode]
   result = Values(kind: vkMulti)
 
-  inc cl.inArgBlock
-
   while true:
     case tree[cr].kind
     of InputNodes:
@@ -1069,7 +1072,6 @@ proc tbArgBlock(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor
       unreachable(tree[cr].kind)
 
   leave(tree, cr)
-  dec cl.inArgBlock
 
   assert stmts.len == 0, "argument block has trailing statements"
 
@@ -1295,18 +1297,27 @@ proc tbScope(tree: TreeWithSource, cl: var TranslateCl, n: MirNode,
              cr: var TreeCursor): CgNode =
   let
     prev = cl.defs.len
+    prevInUnscoped = cl.inUnscoped
+
+  # a scope is entered, meaning that we're no longer in an unscoped context
+  cl.inUnscoped = false
 
   var stmts: seq[CgNode]
   tbList(tree, cl, stmts, cr)
 
   if cl.defs.len > prev:
-    # create a var section for the collected symbols
+    # insert all the lifted defs at the start
     for i in countdown(cl.defs.high, prev):
-      stmts.insert(move cl.defs[i], 0)
+      let (local, info) = cl.defs[i]
+      stmts.insert newStmt(cnkDef, info, [
+        newLocalRef(local, info, cl.locals[local].typ),
+        newEmpty()])
 
     # "pop" the elements that were added as part of this scope:
     cl.defs.setLen(prev)
 
+  cl.inUnscoped = prevInUnscoped
+
   result = toSingleNode(stmts)
 
 proc tbRegion(tree: TreeWithSource, cl: var TranslateCl, prev: sink Values,
@@ -1371,7 +1382,10 @@ proc tbMulti(tree: TreeWithSource, cl: var TranslateCl, cr: var TreeCursor): CgN
   # insert the var section for the collected defs at the start:
   if cl.defs.len > 0:
     for i in countdown(cl.defs.high, 0):
-      nodes.insert(cl.defs[i], 0)
+      let (local, info) = cl.defs[i]
+      nodes.insert newStmt(cnkDef, info, [
+        newLocalRef(local, info, cl.locals[local].typ),
+        newEmpty()])
 
   case nodes.len
   of 0: newEmpty()

tests/arc/topt_cursor.nim

@@ -3,9 +3,10 @@ discard """
   cmd: '''nim c --gc:arc --expandArc:main --expandArc:sio --hint:Performance:off $file'''
   nimout: '''--expandArc: main
 
+var x_cursor
 var :aux_2
 try:
-  var x_cursor = ("hi", 5)
+  x_cursor = ("hi", 5)
   block :label_0:
     if cond:
       x_cursor = [type node](("different", 54))

tests/arc/topt_no_cursor.nim

@@ -11,19 +11,16 @@ doing shady stuff...
   cmd: '''nim c --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
 
-var :aux_3
-var :aux_4
-var :aux_5
 var splat
 splat = splitFile(path)
 result = (
-  :aux_3 = splat.dir
+  var :aux_3 = splat.dir
   wasMoved(splat.dir)
   :aux_3,
-  :aux_4 = splat.name
+  var :aux_4 = splat.name
   wasMoved(splat.name)
   :aux_4,
-  :aux_5 = splat.ext
+  var :aux_5 = splat.ext
   wasMoved(splat.ext)
   :aux_5)
 =destroy(splat)
@@ -62,12 +59,13 @@ result.value = move(lvalue)
 -- end of expandArc ------------------------
 --expandArc: tt
 
+var it_cursor
 var :aux_5
 var :aux_6
 var a
 var :aux_3
 try:
-  var it_cursor = x
+  it_cursor = x
   a = (
     :aux_5 = default()
     =copy(:aux_5, it_cursor.key)
@@ -95,9 +93,10 @@ try:
     while true:
       if not(<(i, L)):
         break :label_0
+      var line
       var splitted
       try:
-        var line = a_cursor[i]
+        line = a_cursor[i]
         splitted = split(line, " ", -1)
         if ==(splitted[0], "opt"):
           var :aux_7 = splitted[1]
@@ -123,11 +122,10 @@ try:
     while true:
       if not(<(i, L)):
         break :label_0
-      var :aux_9
       var sym = a_cursor[i]
       addInterfaceDecl(c,
         var :aux_8 = sym
-        :aux_9 = default()
+        var :aux_9 = default()
         =copy_1(:aux_9, :aux_8)
         :aux_9)
       inc(i, 1)
@@ -141,23 +139,19 @@ try:
   this[].isValid = fileExists(this[].value)
   block :label_0:
     if dirExists(this[].value):
-      var :aux_4
       par = [type node]((
         var :aux_3 = this[].value
-        :aux_4 = default()
+        var :aux_4 = default()
         =copy(:aux_4, :aux_3)
         :aux_4, ""))
       break :label_0
-    var :aux_6
-    var :aux_7
-    var :aux_8
     par = [type node]((parentDir(this[].value),
-      :aux_7 = splitPath(
+      var :aux_7 = splitPath(
         var :aux_5 = this[].value
-        :aux_6 = default()
+        var :aux_6 = default()
         =copy(:aux_6, :aux_5)
         :aux_6)
-      :aux_8 = :aux_7.tail
+      var :aux_8 = :aux_7.tail
       wasMoved(:aux_7.tail)
       :aux_8))
     =destroy(:aux_7)

tests/arc/topt_wasmoved_destroy_pairs.nim

@@ -29,26 +29,23 @@ try:
     while true:
       if not(<(i, b_1)):
         break :label_0
-      var :aux_9
       var i_1_cursor = i
       if ==(i_1_cursor, 2):
         return
       add(a,
-        :aux_9 = default()
+        var :aux_9 = default()
         =copy(:aux_9, x)
         :aux_9)
       inc(i, 1)
   block :label_0:
     if cond:
-      var :aux_10
       add(a,
-        :aux_10 = x
+        var :aux_10 = x
         wasMoved(x)
         :aux_10)
       break :label_0
-    var :aux_11
     add(b,
-      :aux_11 = x
+      var :aux_11 = x
       wasMoved(x)
       :aux_11)
 finally: