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

effects: support callsite @assume_effects annotation #52400

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Dec 5, 2023

@assume_effects on method definitions is very useful, but at some times, its application can be too broad on generic signatures. We've addressed this by defining methods specialized for particular types and applying @assume_effects to them. However, this method has downsides, such as being restricted to pre-set types or increasing nearly identical method definitions.

To remedy this, this commit introduces a support for callsite @assume_effects annotation, enabling finer control over its application. Now the annotations for @ccall are just specific instances of callsite @assume_effects annotation, so it might be a good idea to replace @ccall annotations with callsite @assume_effects annotations.

@nanosoldier runbenchmarks("inference", vs=":master")

@aviatesk aviatesk requested review from vtjnash and Keno December 5, 2023 06:36
@nanosoldier
Copy link
Collaborator

Your job failed.

@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Dec 6, 2023

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Base automatically changed from avi/assume_effects-refactoring to master December 6, 2023 05:03
@aviatesk aviatesk force-pushed the avi/callsite-assume_effects branch 2 times, most recently from 9f4579e to 25eb8fd Compare December 6, 2023 09:06
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

RSLGTM

`@assume_effects` on method definitions is very useful, but at some
times, its application can be too broad on generic signatures. We've
addressed this by defining methods specialized for particular types and
applying `@assume_effects` to them. However, this method has downsides,
such as being restricted to pre-set types or increasing nearly identical
method definitions.

To remedy this, this commit introduces a support for callsite
`@assume_effects` annotation, enabling finer control over its
application. Now the annotations for `@ccall` are just specific
instances of callsite `@assume_effects` annotation, so it might be a
good idea to replace `@ccall` annotations with callsite
`@assume_effects` annotations.
@aviatesk aviatesk merged commit bdbee27 into master Dec 6, 2023
5 of 7 checks passed
@aviatesk aviatesk deleted the avi/callsite-assume_effects branch December 6, 2023 22:06
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.

None yet

3 participants