From a0eca7518223a18f3633150de2c8d3c1c9e71560 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Wed, 4 Mar 2020 14:28:53 +0100 Subject: sink parameter inference for types that have destructors (#13544) * ensure capitalize doesn't take an inferred sink parameter * sink parameter inference: first version, for now disabled. Changed that sink parameters can be consumed multiple times in order to adhere to our spec. * sink inference can now be disabled with .nosinks; sometimes for proc type interop this is required * fixes yet another critical DFA bug * better implementation that also understands if expressions etc * document sink parameter inference and allow for global disabling --- compiler/ast.nim | 9 +++-- compiler/commands.nim | 2 ++ compiler/condsyms.nim | 2 ++ compiler/dfa.nim | 2 ++ compiler/injectdestructors.nim | 12 +++---- compiler/options.nim | 7 ++-- compiler/pragmas.nim | 8 +++-- compiler/sempass2.nim | 22 ++++++++++-- compiler/semstmts.nim | 2 ++ compiler/sinkparameter_inference.nim | 70 ++++++++++++++++++++++++++++++++++++ compiler/wordrecg.nim | 8 ++--- 11 files changed, 124 insertions(+), 20 deletions(-) create mode 100644 compiler/sinkparameter_inference.nim (limited to 'compiler') diff --git a/compiler/ast.nim b/compiler/ast.nim index 3b10a89ea..43ea2b39c 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -229,7 +229,7 @@ type TNodeKinds* = set[TNodeKind] type - TSymFlag* = enum # 40 flags! + TSymFlag* = enum # 41 flags! sfUsed, # read access of sym (for warnings) or simply used sfExported, # symbol is exported from module sfFromGeneric, # symbol is instantiation of a generic; this is needed @@ -238,6 +238,8 @@ type sfGlobal, # symbol is at global scope sfForward, # symbol is forward declared + sfWasForwarded, # symbol had a forward declaration + # (implies it's too dangerous to patch its type signature) sfImportc, # symbol is external; imported sfExportc, # symbol is exported (under a specified name) sfMangleCpp, # mangle as cpp (combines with `sfExportc`) @@ -847,7 +849,7 @@ type position*: int # used for many different things: # for enum fields its position; # for fields its offset - # for parameters its position + # for parameters its position (starting with 0) # for a conditional: # 1 iff the symbol is defined, else 0 # (or not in symbol table) @@ -1865,6 +1867,9 @@ proc isInlineIterator*(typ: PType): bool {.inline.} = proc isClosureIterator*(typ: PType): bool {.inline.} = typ.kind == tyProc and tfIterator in typ.flags and typ.callConv == ccClosure +proc isClosure*(typ: PType): bool {.inline.} = + typ.kind == tyProc and typ.callConv == ccClosure + proc isSinkParam*(s: PSym): bool {.inline.} = s.kind == skParam and (s.typ.kind == tySink or tfHasOwned in s.typ.flags) diff --git a/compiler/commands.nim b/compiler/commands.nim index 0545ee2b0..935d7e2be 100644 --- a/compiler/commands.nim +++ b/compiler/commands.nim @@ -877,6 +877,8 @@ proc processSwitch*(switch, arg: string, pass: TCmdLinePass, info: TLineInfo; localError(conf, info, "unknown Nim version; currently supported values are: {1.0}") of "benchmarkvm": processOnOffSwitchG(conf, {optBenchmarkVM}, arg, pass, info) + of "sinkinference": + processOnOffSwitch(conf, {optSinkInference}, arg, pass, info) of "": # comes from "-" in for example: `nim c -r -` (gets stripped from -) handleStdinInput(conf) else: diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim index d82f4811d..a84b0d878 100644 --- a/compiler/condsyms.nim +++ b/compiler/condsyms.nim @@ -111,3 +111,5 @@ proc initDefines*(symbols: StringTableRef) = # will report the right thing regardless of whether user adds # `-d:nimHasLibFFI` in his user config. defineSymbol("nimHasLibFFIEnabled") + + defineSymbol("nimHasSinkInference") diff --git a/compiler/dfa.nim b/compiler/dfa.nim index b46db9aed..0f23d7f1a 100644 --- a/compiler/dfa.nim +++ b/compiler/dfa.nim @@ -591,6 +591,8 @@ proc genUse(c: var Con; orig: PNode) = of PathKinds1: n = n[1] else: break + if n.kind in nkCallKinds: + gen(c, n) if n.kind == nkSym and n.sym.kind in InterestingSyms: c.code.add Instr(n: orig, kind: use) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index bdd624281..520ba46ce 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -321,8 +321,8 @@ proc destructiveMoveVar(n: PNode; c: var Con): PNode = proc sinkParamIsLastReadCheck(c: var Con, s: PNode) = assert s.kind == nkSym and s.sym.kind == skParam if not isLastRead(s, c): - localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s & - "` is already consumed at " & toFileLineCol(c. graph.config, s.info)) + localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s & + "` is already consumed at " & toFileLineCol(c. graph.config, s.info)) type ProcessMode = enum @@ -498,10 +498,10 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = elif n.kind in {nkBracket, nkObjConstr, nkTupleConstr, nkClosure, nkNilLit} + nkCallKinds + nkLiterals: result = p(n, c, consumed) - elif n.kind == nkSym and isSinkParam(n.sym): + elif n.kind == nkSym and isSinkParam(n.sym) and isLastRead(n, c): # Sinked params can be consumed only once. We need to reset the memory # to disable the destructor which we have not elided - sinkParamIsLastReadCheck(c, n) + #sinkParamIsLastReadCheck(c, n) result = destructiveMoveVar(n, c) elif isAnalysableFieldAccess(n, c.owner) and isLastRead(n, c): # it is the last read, can be sinkArg. We need to reset the memory @@ -687,9 +687,9 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit: result = genSink(c, dest, p(ri, c, consumed)) of nkSym: - if isSinkParam(ri.sym): + if isSinkParam(ri.sym) and isLastRead(ri, c): # Rule 3: `=sink`(x, z); wasMoved(z) - sinkParamIsLastReadCheck(c, ri) + #sinkParamIsLastReadCheck(c, ri) let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) elif ri.sym.kind != skParam and ri.sym.owner == c.owner and diff --git a/compiler/options.nim b/compiler/options.nim index c93fc0648..42406272c 100644 --- a/compiler/options.nim +++ b/compiler/options.nim @@ -39,7 +39,8 @@ type # please make sure we have under 32 options optMemTracker, optLaxStrings, optNilSeqs, - optOldAst + optOldAst, + optSinkInference # 'sink T' inference TOptions* = set[TOption] TGlobalOption* = enum # **keep binary compatible** @@ -200,7 +201,7 @@ type ## the incremental compilation mechanisms ## (+) means "part of the dependency" target*: Target # (+) - linesCompiled*: int # all lines that have been compiled + linesCompiled*: int # all lines that have been compiled options*: TOptions # (+) globalOptions*: TGlobalOptions # (+) macrosToExpand*: StringTableRef @@ -315,7 +316,7 @@ const DefaultOptions* = {optObjCheck, optFieldCheck, optRangeCheck, optBoundsCheck, optOverflowCheck, optAssert, optWarns, optRefCheck, optHints, optStackTrace, optLineTrace, - optTrMacros, optNilCheck, optStyleCheck} + optTrMacros, optNilCheck, optStyleCheck, optSinkInference} DefaultGlobalOptions* = {optThreadAnalysis, optExcessiveStackTrace, optListFullPaths} diff --git a/compiler/pragmas.nim b/compiler/pragmas.nim index 559c1c064..3e89917ed 100644 --- a/compiler/pragmas.nim +++ b/compiler/pragmas.nim @@ -23,7 +23,7 @@ const wExportNims, wExtern, wDeprecated, wNodecl, wError, wUsed, wAlign} ## common pragmas for declarations, to a good approximation procPragmas* = declPragmas + {FirstCallConv..LastCallConv, - wMagic, wNoSideEffect, wSideEffect, wNoreturn, wDynlib, wHeader, + wMagic, wNoSideEffect, wSideEffect, wNoreturn, wNosinks, wDynlib, wHeader, wCompilerProc, wNonReloadable, wCore, wProcVar, wVarargs, wCompileTime, wMerge, wBorrow, wImportCompilerProc, wThread, wAsmNoStackFrame, wDiscardable, wNoInit, wCodegenDecl, @@ -53,7 +53,7 @@ const wLinearScanEnd, wPatterns, wTrMacros, wEffects, wNoForward, wReorder, wComputedGoto, wInjectStmt, wExperimental, wThis, wUsed} lambdaPragmas* = declPragmas + {FirstCallConv..LastCallConv, - wNoSideEffect, wSideEffect, wNoreturn, wDynlib, wHeader, + wNoSideEffect, wSideEffect, wNoreturn, wNosinks, wDynlib, wHeader, wThread, wAsmNoStackFrame, wRaises, wLocks, wTags, wGcSafe, wCodegenDecl} - {wExportNims, wError, wUsed} # why exclude these? typePragmas* = declPragmas + {wMagic, wAcyclic, @@ -357,6 +357,7 @@ proc pragmaToOptions(w: TSpecialWord): TOptions {.inline.} = of wByRef: {optByRef} of wImplicitStatic: {optImplicitStatic} of wPatterns, wTrMacros: {optTrMacros} + of wSinkInference: {optSinkInference} else: {} proc processExperimental(c: PContext; n: PNode) = @@ -889,6 +890,9 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int, of wNoDestroy: noVal(c, it) incl(sym.flags, sfGeneratedOp) + of wNosinks: + noVal(c, it) + incl(sym.flags, sfWasForwarded) of wDynlib: processDynLib(c, it, sym) of wCompilerProc, wCore: diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index 98ef8cf85..af706bee1 100644 --- a/compiler/sempass2.nim +++ b/compiler/sempass2.nim @@ -16,6 +16,7 @@ when defined(useDfa): import dfa import liftdestructors +include sinkparameter_inference #[ Second semantic checking pass over the AST. Necessary because the old way had some inherent problems. Performs: @@ -774,6 +775,10 @@ proc track(tracked: PEffects, n: PNode) = for i in 0.. p: sink T + passToSink(p) # p: sink + ObjConstr(fieldName: p) + [p, q] # array construction + + # Open question: + var local = p # sink parameter? + passToSink(local) + ]# + if optSinkInference notin config.options: return + case arg.kind + of nkSym: + if arg.sym.kind == skParam and + arg.sym.owner == owner and + owner.typ != nil and owner.typ.kind == tyProc and + arg.sym.typ.hasDestructor and + arg.sym.typ.kind notin {tyVar, tySink, tyOwned}: + # Watch out: cannot do this inference for procs with forward + # declarations. + if sfWasForwarded notin owner.flags: + let argType = arg.sym.typ + + let sinkType = newType(tySink, owner) + sinkType.size = argType.size + sinkType.align = argType.align + sinkType.paddingAtEnd = argType.paddingAtEnd + sinkType.add argType + + arg.sym.typ = sinkType + owner.typ[arg.sym.position+1] = sinkType + + #message(config, arg.info, warnUser, + # ("turned '$1' to a sink parameter") % [$arg]) + #echo config $ arg.info, " turned into a sink parameter ", arg.sym.name.s + elif sfWasForwarded notin arg.sym.flags: + # we only report every potential 'sink' parameter only once: + incl arg.sym.flags, sfWasForwarded + message(config, arg.info, hintPerformance, + ("could not turn '$1' to a sink parameter " & + "because '$2' was forward declared") % [arg.sym.name.s, owner.name.s]) + #echo config $ arg.info, " candidate for a sink parameter here" + of nkStmtList, nkStmtListExpr, nkBlockStmt, nkBlockExpr: + if not isEmptyType(arg.typ): + checkForSink(config, owner, arg.lastSon) + of nkIfStmt, nkIfExpr, nkWhen: + for branch in arg: + let value = branch.lastSon + if not isEmptyType(value.typ): + checkForSink(config, owner, value) + of nkCaseStmt: + for i in 1..