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.

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

Merged
merged 1 commit into from
Jul 18, 2025
Merged

fixes inefficient codegen for field return #24874

merged 1 commit into from
Jul 18, 2025

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Apr 14, 2025

fixes #23395
fixes #23395

@ringabout ringabout marked this pull request as ready for review April 14, 2025 16:07
@@ -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()

@arnetheduck
Copy link
Contributor

this remains highly interesting to merge

@Araq
Copy link
Member

Araq commented Jul 18, 2025

I know but I want to solve it differently.

@Araq
Copy link
Member

Araq commented Jul 18, 2025

Oh whatever, I will likely end up with the same logic.

@Araq Araq merged commit 5b5cd7f into devel Jul 18, 2025
21 checks passed
@Araq Araq deleted the pr_own_data branch July 18, 2025 19:30
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
182949 lines; 8.647s; 658.121MiB peakmem

narimiran pushed a commit that referenced this pull request Jul 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inefficient codegen when extracting field from temporary object
3 participants