ARC Analysis in one pass v3 (#17068)

* Analyse last reads all at once

* Integrate firstWrite analysis

* Small cleanup

* Use sets instead of seqs

* Remove instrTargets

* Reap the benefits

* Implement error diagnostics

* Operate on DFA index for lastRead analysis

* Use mgetOrPut

* Cache alias results

This improves performance by a lot, since many
CFG locations map to a single PNode

* Improve performance

* Improve performance

* Cleanup

* Fix #17025

* Grammar

* Expand testcase

compiler/ast.nim

@@ -491,6 +491,8 @@ type
     nfDefaultRefsParam # a default param value references another parameter
                        # the flag is applied to proc default values and to calls
     nfExecuteOnReload  # A top-level statement that will be executed during reloads
+    nfLastRead  # this node is a last read
+    nfFirstWrite# this node is a first write
 
   TNodeFlags* = set[TNodeFlag]
   TTypeFlag* = enum   # keep below 32 for efficiency reasons (now: ~40)
@@ -1014,7 +1016,7 @@ const
                                       nfDotSetter, nfDotField,
                                       nfIsRef, nfIsPtr, nfPreventCg, nfLL,
                                       nfFromTemplate, nfDefaultRefsParam,
-                                      nfExecuteOnReload}
+                                      nfExecuteOnReload, nfLastRead, nfFirstWrite}
   namePos* = 0
   patternPos* = 1    # empty except for term rewriting macros
   genericParamsPos* = 2

compiler/ccgcalls.nim

@@ -298,11 +298,11 @@ proc genArgNoParam(p: BProc, n: PNode, needsTmp = false): Rope =
     initLocExprSingleUse(p, n, a)
     result = rdLoc(withTmpIfNeeded(p, a, needsTmp))
 
-from dfa import instrTargets, InstrTargetKind
+from dfa import aliases, AliasKind
 
 proc potentialAlias(n: PNode, potentialWrites: seq[PNode]): bool =
   for p in potentialWrites:
-    if instrTargets(p, n) != None:
+    if p.aliases(n) != no or n.aliases(p) != no:
       return true
 
 proc skipTrivialIndirections(n: PNode): PNode =

compiler/dfa.nim

@@ -632,8 +632,10 @@ proc aliases*(obj, field: PNode): AliasKind =
     case currFieldPath.kind
     of nkSym:
       if currFieldPath.sym != currObjPath.sym: return no
-    of nkDotExpr, nkCheckedFieldExpr:
+    of nkDotExpr:
       if currFieldPath[1].sym != currObjPath[1].sym: return no
+    of nkCheckedFieldExpr:
+      if currFieldPath[0][1].sym != currObjPath[0][1].sym: return no
     of nkBracketExpr:
       if currFieldPath[1].kind in nkLiterals and currObjPath[1].kind in nkLiterals:
         if currFieldPath[1].intVal != currObjPath[1].intVal:
@@ -642,19 +644,6 @@ proc aliases*(obj, field: PNode): AliasKind =
         result = maybe
     else: assert false # unreachable
 
-type InstrTargetKind* = enum
-  None, Full, Partial
-
-proc instrTargets*(insloc, loc: PNode): InstrTargetKind =
-  case insloc.aliases(loc)
-  of yes:
-    Full    # x -> x; x -> x.f
-  of maybe:
-    Partial # We treat this like a partial write/read
-  elif loc.aliases(insloc) != no:
-    Partial # x.f -> x
-  else: None
-
 proc isAnalysableFieldAccess*(orig: PNode; owner: PSym): bool =
   var n = orig
   while true:

compiler/injectdestructors.nim

@@ -26,7 +26,6 @@ type
     owner: PSym
     g: ControlFlowGraph
     graph: ModuleGraph
-    otherRead: PNode
     inLoop, inSpawn, inLoopCond: int
     uninit: IntSet # set of uninit'ed vars
     uninitComputed: bool
@@ -72,23 +71,56 @@ proc nestedScope(parent: var Scope): Scope =
 proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode): PNode
 proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope; isDecl = false): PNode
 
-proc isLastRead(location: PNode; cfg: ControlFlowGraph; otherRead: var PNode; pc, until: int): int =
+import sets, hashes, tables
-  var pc = pc
+
-  while pc < cfg.len and pc < until:
+proc hash(n: PNode): Hash = hash(cast[pointer](n))
+
+type AliasCache = Table[(PNode, PNode), AliasKind]
+proc aliasesCached(cache: var AliasCache, obj, field: PNode): AliasKind =
+  let key = (obj, field)
+  if not cache.hasKey(key):
+    cache[key] = aliases(obj, field)
+  cache[key]
+
+proc collectLastReads(cfg: ControlFlowGraph; cache: var AliasCache, alreadySeen: var HashSet[PNode], lastReads, potLastReads: var IntSet; pc: var int, until: int) =
+  template aliasesCached(obj, field: PNode): untyped =
+    aliasesCached(cache, obj, field)
+  while pc < until:
     case cfg[pc].kind
     of def:
-      if instrTargets(cfg[pc].n, location) == Full:
+      let potLastReadsCopy = potLastReads
-        # the path leads to a redefinition of 's' --> abandon it.
+      for r in potLastReadsCopy:
-        return high(int)
+        if cfg[pc].n.aliasesCached(cfg[r].n) == yes:
-      elif instrTargets(cfg[pc].n, location) == Partial:
+          # the path leads to a redefinition of 's' --> sink 's'.
+          lastReads.incl r
+          potLastReads.excl r
+        elif cfg[r].n.aliasesCached(cfg[pc].n) != no:
           # only partially writes to 's' --> can't sink 's', so this def reads 's'
-        otherRead = cfg[pc].n
+          # or maybe writes to 's' --> can't sink 's'
-        return -1
+          cfg[r].n.comment = '\n' & $pc
+          potLastReads.excl r
+
+      var alreadySeenThisNode = false
+      for s in alreadySeen:
+        if cfg[pc].n.aliases(s) != no or s.aliases(cfg[pc].n) != no:
+          alreadySeenThisNode = true; break
+      if alreadySeenThisNode: cfg[pc].n.flags.excl nfFirstWrite
+      else: cfg[pc].n.flags.incl nfFirstWrite
+
+      alreadySeen.incl cfg[pc].n
+
       inc pc
     of use:
-      if instrTargets(cfg[pc].n, location) != None:
+      let potLastReadsCopy = potLastReads
-        otherRead = cfg[pc].n
+      for r in potLastReadsCopy:
-        return -1
+        if cfg[pc].n.aliasesCached(cfg[r].n) != no or cfg[r].n.aliasesCached(cfg[pc].n) != no:
+          cfg[r].n.comment = '\n' & $pc
+          potLastReads.excl r
+
+      potLastReads.incl pc
+
+      alreadySeen.incl cfg[pc].n
+
       inc pc
     of goto:
       pc += cfg[pc].dest
@@ -96,99 +128,42 @@ proc isLastRead(location: PNode; cfg: ControlFlowGraph; otherRead: var PNode; pc
       # every branch must lead to the last read of the location:
       var variantA = pc + 1
       var variantB = pc + cfg[pc].dest
-      while variantA != variantB:
+      var potLastReadsA, potLastReadsB = potLastReads
-        if min(variantA, variantB) < 0: return -1
+      var lastReadsA, lastReadsB: IntSet
-        if max(variantA, variantB) >= cfg.len or min(variantA, variantB) >= until:
+      var alreadySeenA, alreadySeenB = alreadySeen
-          break
+      while variantA != variantB and max(variantA, variantB) < cfg.len and min(variantA, variantB) < until:
         if variantA < variantB:
-          variantA = isLastRead(location, cfg, otherRead, variantA, min(variantB, until))
+          collectLastReads(cfg, cache, alreadySeenA, lastReadsA, potLastReadsA, variantA, min(variantB, until))
         else:
-          variantB = isLastRead(location, cfg, otherRead, variantB, min(variantA, until))
+          collectLastReads(cfg, cache, alreadySeenB, lastReadsB, potLastReadsB, variantB, min(variantA, until))
-      pc = min(variantA, variantB)
-  return pc
 
-proc isCursor(n: PNode; c: Con): bool =
+      alreadySeen.incl alreadySeenA + alreadySeenB
-  case n.kind
+
-  of nkSym:
+      # Add those last reads that were turned into last reads on both branches
-    sfCursor in n.sym.flags
+      lastReads.incl lastReadsA * lastReadsB
-  of nkDotExpr:
+      # Add those last reads that were turned into last reads on only one branch,
-    isCursor(n[1], c)
+      # but where the read operation itself also belongs to only that branch
-  of nkCheckedFieldExpr:
+      lastReads.incl (lastReadsA + lastReadsB) - potLastReads
-    isCursor(n[0], c)
+
-  else:
+      let oldPotLastReads = potLastReads
-    false
+      potLastReads = initIntSet()
+
+      potLastReads.incl potLastReadsA + potLastReadsB
+
+      # Remove potential last reads that were invalidated in a branch,
+      # but don't remove those which were turned into last reads on that branch
+      potLastReads.excl ((oldPotLastReads - potLastReadsA) - lastReadsA)
+      potLastReads.excl ((oldPotLastReads - potLastReadsB) - lastReadsB)
+
+      pc = min(variantA, variantB)
 
 proc isLastRead(n: PNode; c: var Con): bool =
-  # first we need to search for the instruction that belongs to 'n':
-  var instr = -1
   let m = dfa.skipConvDfa(n)
-  if m.kind == nkSym and sfSingleUsedTemp in m.sym.flags: return true
+  (m.kind == nkSym and sfSingleUsedTemp in m.sym.flags) or nfLastRead in m.flags
-
-  for i in 0..<c.g.len:
-    # This comparison is correct and MUST not be ``instrTargets``:
-    if c.g[i].kind == use and c.g[i].n == m:
-      if instr < 0:
-        instr = i
-        break
-
-  dbg: echo "starting point for ", n, " is ", instr, " ", n.kind
-
-  if instr < 0: return false
-  # we go through all paths beginning from 'instr+1' and need to
-  # ensure that we don't find another 'use X' instruction.
-  if instr+1 >= c.g.len: return true
-
-  c.otherRead = nil
-  result = isLastRead(n, c.g, c.otherRead, instr+1, int.high) >= 0
-  dbg: echo "ugh ", c.otherRead.isNil, " ", result
-
-proc isFirstWrite(location: PNode; cfg: ControlFlowGraph; pc, until: int): int =
-  var pc = pc
-  while pc < until:
-    case cfg[pc].kind
-    of def:
-      if instrTargets(cfg[pc].n, location) != None:
-        # a definition of 's' before ours makes ours not the first write
-        return -1
-      inc pc
-    of use:
-      if instrTargets(cfg[pc].n, location) != None:
-        return -1
-      inc pc
-    of goto:
-      pc += cfg[pc].dest
-    of fork:
-      # every branch must not contain a def/use of our location:
-      var variantA = pc + 1
-      var variantB = pc + cfg[pc].dest
-      while variantA != variantB:
-        if min(variantA, variantB) < 0: return -1
-        if max(variantA, variantB) > until:
-          break
-        if variantA < variantB:
-          variantA = isFirstWrite(location, cfg, variantA, min(variantB, until))
-        else:
-          variantB = isFirstWrite(location, cfg, variantB, min(variantA, until))
-      pc = min(variantA, variantB)
-  return pc
 
 proc isFirstWrite(n: PNode; c: var Con): bool =
-  # first we need to search for the instruction that belongs to 'n':
-  var instr = -1
   let m = dfa.skipConvDfa(n)
-
+  nfFirstWrite in m.flags
-  for i in countdown(c.g.len-1, 0): # We search backwards here to treat loops correctly
-    if c.g[i].kind == def and c.g[i].n == m:
-      if instr < 0:
-        instr = i
-        break
-
-  if instr < 0: return false
-  # we go through all paths going to 'instr' and need to
-  # ensure that we don't find another 'def/use X' instruction.
-  if instr == 0: return true
-
-  result = isFirstWrite(n, c.g, 0, instr) >= 0
 
 proc initialized(code: ControlFlowGraph; pc: int,
                  init, uninit: var IntSet; until: int): int =
@@ -228,20 +203,33 @@ proc initialized(code: ControlFlowGraph; pc: int,
       inc pc
   return pc
 
+proc isCursor(n: PNode; c: Con): bool =
+  case n.kind
+  of nkSym:
+    sfCursor in n.sym.flags
+  of nkDotExpr:
+    isCursor(n[1], c)
+  of nkCheckedFieldExpr:
+    isCursor(n[0], c)
+  else:
+    false
+
 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)
 
+from strutils import parseInt
+
 proc checkForErrorPragma(c: Con; t: PType; ri: PNode; opname: string) =
   var m = "'" & opname & "' is not available for type <" & typeToString(t) & ">"
   if (opname == "=" or opname == "=copy") and ri != nil:
     m.add "; requires a copy because it's not the last read of '"
     m.add renderTree(ri)
     m.add '\''
-    if c.otherRead != nil:
+    if ri.comment.startsWith('\n'):
       m.add "; another read is done here: "
-      m.add c.graph.config $ c.otherRead.info
+      m.add c.graph.config $ c.g[parseInt(ri.comment[1..^1])].n.info
     elif ri.kind == nkSym and ri.sym.kind == skParam and not isSinkType(ri.sym.typ):
       m.add "; try to make "
       m.add renderTree(ri)
@@ -355,7 +343,6 @@ proc genCopy(c: var Con; dest, ri: PNode): PNode =
   let t = dest.typ
   if tfHasOwned in t.flags and ri.kind != nkNilLit:
     # try to improve the error message here:
-    if c.otherRead == nil: discard isLastRead(ri, c)
     c.checkForErrorPragma(t, ri, "=copy")
   result = c.genCopyNoCheck(dest, ri)
 
@@ -1097,6 +1084,24 @@ proc injectDestructorCalls*(g: ModuleGraph; idgen: IdGenerator; owner: PSym; n:
   if optCursorInference in g.config.options:
     computeCursors(owner, n, g)
 
+  block:
+    var cache = initTable[(PNode, PNode), AliasKind]()
+    var alreadySeen: HashSet[PNode]
+    var lastReads, potLastReads: IntSet
+    var pc = 0
+    collectLastReads(c.g, cache, alreadySeen, lastReads, potLastReads, pc, c.g.len)
+    lastReads.incl potLastReads
+    var lastReadTable: Table[PNode, seq[int]]
+    for position, node in c.g:
+      if node.kind == use:
+        lastReadTable.mgetOrPut(node.n, @[]).add position
+    for node, positions in lastReadTable:
+      var allPositionsLastRead = true
+      for p in positions:
+        if p notin lastReads: allPositionsLastRead = false; break
+      if allPositionsLastRead:
+        node.flags.incl nfLastRead
+
   var scope: Scope
   let body = p(n, c, scope, normal)
 

compiler/renderer.nim

@@ -296,11 +296,11 @@ proc popAllComs(g: var TSrcGen) =
 const
   Space = " "
 
-proc shouldRenderComment(g: var TSrcGen, n: PNode): bool =
+proc shouldRenderComment(g: TSrcGen): bool {.inline.} =
-  result = false
+  (renderNoComments notin g.flags or renderDocComments in g.flags)
-  if n.comment.len > 0:
+
-    result = (renderNoComments notin g.flags) or
+proc shouldRenderComment(g: TSrcGen, n: PNode): bool {.inline.} =
-        (renderDocComments in g.flags)
+  shouldRenderComment(g) and n.comment.len > 0
 
 proc gcom(g: var TSrcGen, n: PNode) =
   assert(n != nil)
@@ -430,7 +430,7 @@ proc lsons(g: TSrcGen; n: PNode, start: int = 0, theEnd: int = - 1): int =
 proc lsub(g: TSrcGen; n: PNode): int =
   # computes the length of a tree
   if isNil(n): return 0
-  if n.comment.len > 0: return MaxLineLen + 1
+  if shouldRenderComment(g, n): return MaxLineLen + 1
   case n.kind
   of nkEmpty: result = 0
   of nkTripleStrLit:
@@ -598,7 +598,7 @@ proc gcommaAux(g: var TSrcGen, n: PNode, ind: int, start: int = 0,
     if c:
       if g.tokens.len > oldLen:
         putWithSpace(g, separator, $separator)
-      if hasCom(n[i]):
+      if shouldRenderComment(g) and hasCom(n[i]):
         gcoms(g)
         optNL(g, ind)
 
@@ -639,7 +639,7 @@ proc gsection(g: var TSrcGen, n: PNode, c: TContext, kind: TokType,
   dedent(g)
 
 proc longMode(g: TSrcGen; n: PNode, start: int = 0, theEnd: int = - 1): bool =
-  result = n.comment.len > 0
+  result = shouldRenderComment(g, n)
   if not result:
     # check further
     for i in start..n.len + theEnd:
@@ -980,7 +980,7 @@ proc gsub(g: var TSrcGen, n: PNode, c: TContext) =
   if isNil(n): return
   var
     a: TContext
-  if n.comment.len > 0: pushCom(g, n)
+  if shouldRenderComment(g, n): pushCom(g, n)
   case n.kind                 # atoms:
   of nkTripleStrLit: put(g, tkTripleStrLit, atom(g, n))
   of nkEmpty: discard

tests/arc/t17025.nim

@@ -0,0 +1,56 @@
+discard """
+  cmd: "nim c --gc:arc $file"
+  output: '''
+{"Package": {"name": "hello"}, "Author": {"name": "name", "qq": "123456789", "email": "email"}}
+hello
+name
+123456789
+email
+hello
+name2
+987654321
+liame
+'''
+"""
+
+import parsecfg, streams, tables
+
+const cfg = """[Package]
+name=hello
+[Author]
+name=name
+qq=123456789
+email="email""""
+
+proc main() =
+    let stream = newStringStream(cfg)
+    let dict = loadConfig(stream)
+    var pname = dict.getSectionValue("Package","name")
+    var name = dict.getSectionValue("Author","name")
+    var qq = dict.getSectionValue("Author","qq")
+    var email = dict.getSectionValue("Author","email")
+    echo dict[]
+    echo pname & "\n" & name & "\n" & qq & "\n" & email
+    stream.close()
+
+main()
+
+proc getDict(): OrderedTableRef[string, OrderedTableRef[string, string]] =
+    result = newOrderedTable[string, OrderedTableRef[string, string]]()
+    result["Package"] = newOrderedTable[string, string]()
+    result["Package"]["name"] = "hello"
+    result["Author"] = newOrderedTable[string, string]()
+    result["Author"]["name"] = "name2"
+    result["Author"]["qq"] = "987654321"
+    result["Author"]["email"] = "liame"
+
+proc main2() =
+    let dict = getDict()
+    var pname = dict.getSectionValue("Package","name")
+    var name = dict.getSectionValue("Author","name")
+    var qq = dict.getSectionValue("Author","qq")
+    var email = dict.getSectionValue("Author","email")
+    echo pname & "\n" & name & "\n" & qq & "\n" & email
+
+main2()
+

tests/arc/tmovebug.nim

@@ -93,7 +93,6 @@ destroy
 destroy
 destroy
 sink
-sink
 destroy
 copy
 (f: 1)
@@ -687,7 +686,7 @@ caseNotAConstant()
 
 proc potentialSelfAssign(i: var int) =
   var a: array[2, OO]
-  a[i] = OO(f: 1)
+  a[i] = OO(f: 1) # turned into a memcopy
   a[1] = OO(f: 2)
   a[i+1] = a[i] # This must not =sink, but =copy
   inc i
@@ -744,3 +743,30 @@ proc partToWholeUnownedRef =
 
 partToWholeUnownedRef()
 
+
+#--------------------------------------------------------------------
+# test that nodes that get copied during the transformation
+# (like dot exprs) don't loose their firstWrite/lastRead property
+
+type
+  OOO = object
+    initialized: bool
+
+  C = object
+    o: OOO
+
+proc `=destroy`(o: var OOO) =
+  doAssert o.initialized, "OOO was destroyed before initialization!"
+
+proc initO(): OOO =
+  OOO(initialized: true)
+
+proc initC(): C =
+  C(o: initO())
+
+proc pair(): tuple[a: C, b: C] =
+  result.a = initC() # <- when firstWrite tries to find this node to start its analysis it fails, because injectdestructors uses copyTree/shallowCopy
+  result.b = initC()
+
+discard pair()
+

tests/arc/topt_no_cursor.nim

@@ -56,7 +56,7 @@ _ = (
   blitTmp, ";")
 lvalue = _[0]
 lnext = _[1]
-`=sink`(result.value, move lvalue)
+result.value = move lvalue
 `=destroy`(lnext)
 `=destroy_1`(lvalue)
 -- end of expandArc ------------------------