Skip to content
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

Jump to bottom

implement =dup hook eliminating wasMoved and =copy pairs #21586

Merged
merged 12 commits into from May 6, 2023
Merged

implement =dup hook eliminating wasMoved and =copy pairs #21586

merged 12 commits into from May 6, 2023

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Mar 30, 2023 •
edited 

type
  Ref = ref object
    id: int

  RefCustom = object
    id: ptr int

proc inc(x: sink Ref) =
  inc x.id

proc inc(x: sink RefCustom) =
  inc x.id[]

proc `=dup`(x: var RefCustom): RefCustom =
  result.id = x.id

proc foo =
  var x = Ref(id: 8)
  inc(x)
  inc(x)
  var id = 777
  var s = RefCustom(id: addr id)
  inc s
  inc s

foo()
--expandArc: foo
var
  x
  :tmpD
  s
  :tmpD_1
x = Ref(id: 8)
inc:
  :tmpD = `=dup`(x)
  :tmpD
inc:
  let blitTmp = x
  blitTmp
var id_1 = 777
s = RefCustom(id_2: addr(id_1))
inc_1 :
  :tmpD_1 = `=dup`(s)
  :tmpD_1
inc_1 :
  let blitTmp_1 = s
  blitTmp_1
-- end of expandArc ------------------------

Araq
Araq reviewed Mar 30, 2023
View reviewed changes
lib/system.nim Outdated Show resolved Hide resolved
Araq
Araq reviewed Mar 30, 2023
View reviewed changes
lib/system.nim Outdated Show resolved Hide resolved
Araq
Araq reviewed Mar 30, 2023
View reviewed changes
tests/arc/tdup.nim Outdated Show resolved Hide resolved
@ringabout

This comment was marked as off-topic.

Sign in to view
@ringabout ringabout marked this pull request as ready for review May 2, 2023 14:08
Araq
Araq reviewed May 6, 2023
View reviewed changes
compiler/liftdestructors.nim
@@ -851,6 +869,8 @@ proc ownedClosureOp(c: var TLiftCtx; t: PType; body, x, y: PNode) =
of attachedDeepCopy: assert(false, "cannot happen")
of attachedTrace: discard
of attachedWasMoved: body.add genBuiltin(c, mWasMoved, "wasMoved", x)
of attachedDup:
assert false, "cannot happen"
Copy link
Member

Choose a reason for hiding this comment

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

In follow-up PRs we should lift =dup like other hooks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Araq Hi, may I ask what's the benefit of lifting an operation? Like it can be used by other hooks or something?

Copy link
Member

Choose a reason for hiding this comment

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

The =dup hook is more efficient than the wasMoved(tmp); =sink(tmp, y) combination in principle. If you lift it so that more types are affected it remains to be more efficient.

@Araq Araq merged commit b562e1e into devel May 6, 2023
21 checks passed
@Araq Araq deleted the pr_dup branch May 6, 2023 19:36
@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from b562e1e

Hint: mm: orc; opt: speed; options: -d:release
167084 lines; 10.245s; 612.969MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request May 15, 2023
@ringabout @capocasa
implement =dup hook eliminating wasMoved and =copy pairs (nim-l…
c8983a6 
…ang#21586)

* import `=dup` hook eliminating `wasMoved` and `=copy` pairs

* add dup

* add a test for dup

* fixes documentation

* fixes signature

* resolve comments

* fixes tests

* fixes tests

* clean up
capocasa pushed a commit to capocasa/Nim that referenced this pull request May 16, 2023
@ringabout @capocasa
implement =dup hook eliminating wasMoved and =copy pairs (nim-l…
73a11b5 
…ang#21586)

* import `=dup` hook eliminating `wasMoved` and `=copy` pairs

* add dup

* add a test for dup

* fixes documentation

* fixes signature

* resolve comments

* fixes tests

* fixes tests

* clean up
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
@ringabout @bung87
implement =dup hook eliminating wasMoved and =copy pairs (nim-l…
cf76a29 
…ang#21586)

* import `=dup` hook eliminating `wasMoved` and `=copy` pairs

* add dup

* add a test for dup

* fixes documentation

* fixes signature

* resolve comments

* fixes tests

* fixes tests

* clean up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ringabout @Araq