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.

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

fix annotations on sym_in #51573

Merged
merged 1 commit into from
Oct 4, 2023
Merged

fix annotations on sym_in #51573

merged 1 commit into from
Oct 4, 2023

Conversation

JeffBezanson
Copy link
Sponsor Member

This seems to be the right combination of annotations to fix both #50562 and an inference regression in PropertyDicts.jl on the 1.10 release branch. When backported the @noinline should be restored for 1.10.

@JeffBezanson JeffBezanson added performance Must go faster backport 1.10 Change should be backported to the 1.10 release labels Oct 3, 2023
Copy link
Sponsor Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

Just for future reference, could you explain the rationale behind adding @nospecialize in #36596, even though we're now considering its removal? As for the use of @noinline here, the reasoning can be found in this comment.

@JeffBezanson
Copy link
Sponsor Member Author

I assume we added it just because it doesn't seem necessary to re-compile the function for every length of tuple --- if the tuples are heap allocated. But now that we can stack allocate them there is some value to it.

@vtjnash vtjnash merged commit f7e8f92 into master Oct 4, 2023
6 of 8 checks passed
@vtjnash vtjnash deleted the jb/sym_in branch October 4, 2023 21:16
KristofferC pushed a commit that referenced this pull request Oct 10, 2023
This seems to be the right combination of annotations to fix both #50562
and an inference regression in PropertyDicts.jl on the 1.10 release
branch. When backported the `@noinline` should be restored for 1.10.

(cherry picked from commit f7e8f92)
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants