Skip to content

fixes inefficient codegen for field return #24874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
+26 −3
Open

fixes inefficient codegen for field return #24874

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions compiler/injectdestructors.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ when false:
proc isLastReadImpl(n: PNode; c: var Con; scope: var Scope): bool =
let root = parampatterns.exprRoot(n, allowCalls=false)
if root == nil: return false
elif sfSingleUsedTemp in root.flags: return true

var s = addr(scope)
while s != nil:
Expand Down Expand Up @@ -167,8 +168,7 @@ proc isLastRead(n: PNode; c: var Con; s: var Scope): bool =
if not hasDestructor(c, n.typ) and (n.typ.kind != tyObject or isTrival(getAttachedOp(c.graph, n.typ, attachedAsgn))): return true

let m = skipConvDfa(n)
result = (m.kind == nkSym and sfSingleUsedTemp in m.sym.flags) or
isLastReadImpl(n, c, s)
result = isLastReadImpl(n, c, s)

proc isFirstWrite(n: PNode; c: var Con): bool =
let m = skipConvDfa(n)
Expand Down Expand Up @@ -1122,6 +1122,25 @@ proc genFieldAccessSideEffects(c: var Con; s: var Scope; dest, ri: PNode; flags:
var snk = c.genSink(s, dest, newAccess, flags)
result = newTree(nkStmtList, v, snk, c.genWasMoved(newAccess))

proc ownsData(c: var Con; s: var Scope; orig: PNode; flags: set[MoveOrCopyFlag]): PNode =
var n = orig
while true:
case n.kind
of nkDotExpr, nkCheckedFieldExpr, nkBracketExpr:
n = n[0]
else:
break
if n.kind in nkCallKinds and n.typ != nil and hasDestructor(c, n.typ):
result = newNodeIT(nkStmtListExpr, orig.info, orig.typ)
let tmp = c.getTemp(s, n.typ, n.info)
tmp.sym.flags.incl sfSingleUsedTemp
result.add newTree(nkFastAsgn, tmp, copyTree(n))
s.final.add c.genDestroy(tmp)
n[] = tmp[]
result.add copyTree(orig)
else:
result = nil

proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, flags: set[MoveOrCopyFlag] = {}): PNode =
var ri = ri
var isEnsureMove = 0
Expand Down Expand Up @@ -1205,7 +1224,11 @@ proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope, flags: set[MoveOrCopy
of nkRaiseStmt:
result = pRaiseStmt(ri, c, s)
else:
if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c, s) and
let isOwnsData = ownsData(c, s, ri2, flags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary logic. Shouldn't isAnalysableFieldAccess etc be able to handle this? I think there is a real bug to be fixed here somewhere, not more custom logic to be added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't isAnalysableFieldAccess, isLastRead etc. analysis based on nkSym? What's the rule that it should handle callkinds in f().s? Should I treat the callkinds as the last read?

On the other hand, the # Rule 3: =sink(x, z); wasMoved(z) branch is not correct for f().s, because f() may have side effects. We need to create a temp for f() and clean up the potential memory of f()


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):
# Rule 3: `=sink`(x, z); wasMoved(z)
let snk = c.genSink(s, dest, ri, flags)
Expand Down