From 0b9859df3b086ae584109851ecc2b188fe8fba82 Mon Sep 17 00:00:00 2001 From: Araq Date: Tue, 9 Jun 2026 11:52:05 +0200 Subject: [PATCH] yet another attempt at #25333 --- compiler/astdef.nim | 8 +++- compiler/dfa.nim | 48 ++++++++++++++++++++++++ compiler/ic/enum2nif.nim | 8 ++-- compiler/injectdestructors.nim | 59 +++++++----------------------- compiler/lambdalifting.nim | 67 +++++++++++++++++++++++++++++++++- compiler/transf.nim | 4 ++ 6 files changed, 140 insertions(+), 54 deletions(-) diff --git a/compiler/astdef.nim b/compiler/astdef.nim index 6a7b4d7788..19d6d562f6 100644 --- a/compiler/astdef.nim +++ b/compiler/astdef.nim @@ -319,8 +319,12 @@ 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 + nfLastUse # this node is a last use (a "last read" that was computed + # before lambda lifting; the move analyser trusts it for + # captured variables turned into `env[].field` accesses). + # Deliberately not serialized through IC: it is a derived + # property recomputed each run. nfHasComment # node has a comment nfSkipFieldChecking # node skips field visable checking nfDisabledOpenSym # temporary: node should be nkOpenSym but cannot @@ -869,7 +873,7 @@ const nfDotSetter, nfDotField, nfIsRef, nfIsPtr, nfPreventCg, nfLL, nfFromTemplate, nfDefaultRefsParam, - nfExecuteOnReload, nfLastRead, + nfExecuteOnReload, nfFirstWrite, nfSkipFieldChecking, nfDisabledOpenSym, nfLazyType} namePos* = 0 diff --git a/compiler/dfa.nim b/compiler/dfa.nim index c946c30b74..787e344c11 100644 --- a/compiler/dfa.nim +++ b/compiler/dfa.nim @@ -489,3 +489,51 @@ proc constructCfg*(s: PSym; body: PNode; root: PSym): ControlFlowGraph = shallowCopy(result, c.code) when false: optimizeJumps result + +proc isLastReadOfRoot*(g: ControlFlowGraph; n: PNode; start: int; + otherUsage: var TLineInfo): bool = + ## Walks the control flow graph `g` starting at instruction index `start` + ## (which must be the index *after* the `use` of `n`) and returns whether + ## `n` is the last read of its location. On a conflicting later use/def the + ## location of the offending instruction is stored in `otherUsage`. + ## + ## This is the shared core of the move analyser; it is used both by + ## `injectdestructors` (post lambda lifting) and by the pre-lifting marking + ## pass in `lambdalifting` that stamps `nfLastUse` on captured variables. + var pcs = @[start] + var marked = initIntSet() + result = true + while pcs.len > 0: + var pc = pcs.pop() + if not marked.contains(pc): + let oldPc = pc + while pc < g.len: + case g[pc].kind + of loop: + let back = pc + g[pc].dest + if not marked.containsOrIncl(back): + pc = back + else: + break + of goto: + pc = pc + g[pc].dest + of fork: + if not marked.contains(pc+1): + pcs.add pc + 1 + pc = pc + g[pc].dest + of use: + if g[pc].n.aliases(n) != no or n.aliases(g[pc].n) != no: + otherUsage = g[pc].n.info + return false + inc pc + of def: + if g[pc].n.aliases(n) == yes: + # the path leads to a redefinition of 's' --> sink 's'. + break + elif n.aliases(g[pc].n) != no: + # only partially writes to 's' --> can't sink 's', so this def reads 's' + # or maybe writes to 's' --> can't sink 's' + otherUsage = g[pc].n.info + return false + inc pc + marked.incl oldPc diff --git a/compiler/ic/enum2nif.nim b/compiler/ic/enum2nif.nim index 4b7860fc96..35ba4b0cdc 100644 --- a/compiler/ic/enum2nif.nim +++ b/compiler/ic/enum2nif.nim @@ -1467,12 +1467,12 @@ proc genFlags*(s: set[TNodeFlag]; dest: var string) = of nfDefaultParam: dest.add "d1" of nfDefaultRefsParam: dest.add "d2" of nfExecuteOnReload: dest.add "o" - of nfLastRead: dest.add "l0" of nfFirstWrite: dest.add "w" + of nfLastUse: dest.add "u" of nfHasComment: dest.add "h" of nfSkipFieldChecking: dest.add "s0" of nfDisabledOpenSym: dest.add "d3" - of nfLazyType: dest.add "l1" + of nfLazyType: dest.add "l0" proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] = @@ -1513,9 +1513,6 @@ proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] = of 'i': result.incl nfIsRef of 'l': if i+1 < s.len and s[i+1] == '0': - result.incl nfLastRead - inc i - elif i+1 < s.len and s[i+1] == '1': result.incl nfLazyType inc i else: result.incl nfLL @@ -1533,6 +1530,7 @@ proc parse*(t: typedesc[TNodeFlag]; s: string): set[TNodeFlag] = inc i else: result.incl nfSem of 't': result.incl nfTransf + of 'u': result.incl nfLastUse of 'w': result.incl nfFirstWrite else: discard inc i diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 2b5ae6421e..57d2f4e2ac 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -121,47 +121,7 @@ proc isLastReadImpl(n: PNode; c: var Con; scope: var Scope): bool = inc j c.otherUsage = unknownLineInfo if j < c.g.len: - var pcs = @[j+1] - var marked = initIntSet() - result = true - while pcs.len > 0: - var pc = pcs.pop() - if not marked.contains(pc): - let oldPc = pc - while pc < c.g.len: - dbg: - echo "EXEC ", c.g[pc].kind, " ", pc, " ", n - when false: - inc perfCounters[c.g[pc].kind] - case c.g[pc].kind - of loop: - let back = pc + c.g[pc].dest - if not marked.containsOrIncl(back): - pc = back - else: - break - of goto: - pc = pc + c.g[pc].dest - of fork: - if not marked.contains(pc+1): - pcs.add pc + 1 - pc = pc + c.g[pc].dest - of use: - if c.g[pc].n.aliases(n) != no or n.aliases(c.g[pc].n) != no: - c.otherUsage = c.g[pc].n.info - return false - inc pc - of def: - if c.g[pc].n.aliases(n) == yes: - # the path leads to a redefinition of 's' --> sink 's'. - break - elif n.aliases(c.g[pc].n) != no: - # only partially writes to 's' --> can't sink 's', so this def reads 's' - # or maybe writes to 's' --> can't sink 's' - c.otherUsage = c.g[pc].n.info - return false - inc pc - marked.incl oldPc + result = isLastReadOfRoot(c.g, n, j+1, c.otherUsage) else: result = false @@ -170,8 +130,16 @@ template hasDestructorOrAsgn(c: var Con, typ: PType): bool = hasDestructor(c, typ) or (c.graph.config.selectedGC in {gcArc, gcOrc, gcYrc, gcAtomicArc} and typ.kind == tyObject and not isTrivial(getAttachedOp(c.graph, typ, attachedAsgn))) +proc isLastUseMarked(n: PNode): bool = + ## A captured variable read that the move analyser proved was a last read + ## *before* lambda lifting (see `markLastUses` in lambdalifting). After + ## lifting it became an `env[].field` access that we can no longer analyse + ## here, so we trust the recorded verdict. + nfLastUse in skipConvDfa(n).flags + proc isLastRead(n: PNode; c: var Con; s: var Scope): bool = if not hasDestructorOrAsgn(c, n.typ): return true + if isLastUseMarked(n): return true let m = skipConvDfa(n) result = isLastReadImpl(n, c, s) @@ -834,7 +802,8 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode; tmpFlags = {sfSing elif n.kind in {nkBracket, nkObjConstr, nkTupleConstr, nkClosure, nkNilLit} + nkCallKinds + nkLiterals: result = p(n, c, s, consumed) - elif ((n.kind == nkSym and isSinkParam(n.sym)) or isAnalysableFieldAccess(n, c.owner)) and + elif ((n.kind == nkSym and isSinkParam(n.sym)) or isAnalysableFieldAccess(n, c.owner) or + isLastUseMarked(n)) and isLastRead(n, c, s) and not (n.kind == nkSym and isCursor(n)): # Sinked params can be consumed only once. We need to reset the memory # to disable the destructor which we have not elided @@ -1077,7 +1046,7 @@ proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode; tmpFlags = {sfSing for i in 1 ..< n.len: result[i] = n[i] if mode == sinkArg and hasDestructor(c, n.typ): - if isAnalysableFieldAccess(n, c.owner) and isLastRead(n, c, s): + if (isAnalysableFieldAccess(n, c.owner) or isLastUseMarked(n)) and isLastRead(n, c, s): s.wasMoved.add c.genWasMoved(n) else: result = passCopyToSink(result, c, s) @@ -1278,8 +1247,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, flags: set[MoveOrCopy if isOwnsData != nil: result = moveOrCopy(dest, isOwnsData, c, s, flags) - elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c, s) and - canBeMoved(c, dest.typ): + elif (isAnalysableFieldAccess(ri, c.owner) or isLastUseMarked(ri)) and + isLastRead(ri, c, s) and canBeMoved(c, dest.typ): # Rule 3: `=sink`(x, z); wasMoved(z) let snk = c.genSink(s, dest, ri, flags) result = newTree(nkStmtList, snk, c.genWasMoved(ri)) diff --git a/compiler/lambdalifting.nim b/compiler/lambdalifting.nim index a94db43211..ac018dafaa 100644 --- a/compiler/lambdalifting.nim +++ b/compiler/lambdalifting.nim @@ -12,7 +12,7 @@ import options, ast, astalgo, msgs, idents, renderer, types, magicsys, lowerings, modulegraphs, lineinfos, - transf, liftdestructors, typeallowed + transf, liftdestructors, typeallowed, dfa, aliasanalysis import std/[strutils, tables, intsets] @@ -553,7 +553,10 @@ proc accessViaEnvParam(g: ModuleGraph; n: PNode; owner: PSym): PNode = assert obj.kind == tyObject let field = getFieldFromObj(obj, s) if field != nil: - return rawIndirectAccess(access, field, n.info) + result = rawIndirectAccess(access, field, n.info) + # carry the pre-lifting move-analysis verdict onto the env access: + if nfLastUse in n.flags: result.flags.incl nfLastUse + return result let upField = lookupInRecord(obj.n, getIdent(g.cache, upName)) if upField == nil: break access = rawIndirectAccess(access, upField, n.info) @@ -718,6 +721,8 @@ proc accessViaEnvVar(n: PNode; owner: PSym; d: var DetectionPass; let field = getFieldFromObj(obj, n.sym) if field != nil: result = rawIndirectAccess(access, field, n.info) + # carry the pre-lifting move-analysis verdict onto the env access: + if nfLastUse in n.flags: result.flags.incl nfLastUse else: localError(d.graph.config, n.info, "internal error: not part of closure object type") result = n @@ -874,6 +879,64 @@ proc semCaptureSym*(s, owner: PSym) = # since the analysis is not entirely correct, we don't set 'tfCapturesEnv' # here +proc collectRoots(n: PNode; fn: PSym; roots: var seq[PSym]; seen: var IntSet) = + case n.kind + of nkSym: + let s = n.sym + # captured == defined in an enclosing routine; `fn`'s own locals/params + # (owner == fn) are not lifted into a shared env this way. + if s.owner != fn and interestingVar(s) and hasDestructor(s.typ) and + not seen.containsOrIncl(s.id): + roots.add s + of nkEmpty..pred(nkSym), succ(nkSym)..nkNilLit, nkTypeSection, + nkProcDef, nkFuncDef, nkMethodDef, nkConverterDef, nkMacroDef, + nkIteratorDef, nkTemplateDef, nkLambdaKinds: + # do not descend into nested routine definitions: their captured + # variables get their own marking pass when they are transformed. + discard + else: + for child in n: collectRoots(child, fn, roots, seen) + +proc markLastUses*(g: ModuleGraph; fn: PSym; body: PNode) = + ## Move analysis *before* lambda lifting. A variable defined in an enclosing + ## routine and captured by `fn` is, at this point, still an ordinary + ## non-escaping local that the move analyser can reason about. We compute its + ## last reads inside `fn`'s body and stamp them with `nfLastUse`. After + ## lifting these reads become `env[].field` accesses through a shared `ref`; + ## the move analyser in `injectdestructors` can no longer prove they are last + ## reads (the env may be aliased), so it trusts the `nfLastUse` flag instead + ## (bug #25333). The flag is carried onto the rewritten access by + ## `accessViaEnvParam`/`accessViaEnvVar`. + ## + ## Must run before `transformClosureIterator` turns the body into a state + ## machine (whose back-edges would defeat the analysis) and before the + ## enclosing lifting pass rewrites the captured reads. + # Only *closure iterators* are safe: their environment is created per + # instantiation and the body runs to completion exactly once, so a statically + # last read is also dynamically the last read. A regular closure's env + # persists across invocations, so the body may run again and re-read the + # variable; moving it out on a "last read" would corrupt the next call. Hence + # we must not mark captured reads in regular closures (bug #25333). + if not fn.isIterator: return + if optSeqDestructors notin g.config.globalOptions: return + + var roots: seq[PSym] = @[] + var seen = initIntSet() + collectRoots(body, fn, roots, seen) + if roots.len == 0: return + + for root in roots: + let cfg = constructCfg(fn, body, root) + var otherUsage = unknownLineInfo + for j in 0 ..< cfg.len: + if cfg[j].kind == use: + let m = skipConvDfa(cfg[j].n) + # only whole-variable reads can be safely propagated through lifting; + # partial accesses (`v.field`, `v[i]`) keep the old behaviour. + if m.kind == nkSym and m.sym == root and + isLastReadOfRoot(cfg, cfg[j].n, j+1, otherUsage): + m.flags.incl nfLastUse + proc liftIterToProc*(g: ModuleGraph; fn: PSym; body: PNode; ptrType: PType; idgen: IdGenerator): PNode = var d = initDetectionPass(g, fn, idgen) diff --git a/compiler/transf.nim b/compiler/transf.nim index f131c862f0..2fe76288f3 100644 --- a/compiler/transf.nim +++ b/compiler/transf.nim @@ -1392,6 +1392,10 @@ proc transformBody*(g: ModuleGraph; idgen: IdGenerator; prc: PSym; flags: Transf liftDefer(c, result) result = liftLocalsIfRequested(prc, result, g.cache, g.config, c.idgen) + # mark last reads of captured variables before the closure-iterator state + # machine and the enclosing lifting pass rewrite them (bug #25333): + markLastUses(g, prc, result) + if prc.isIterator: result = g.transformClosureIterator(c.idgen, prc, result)