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

Add @inline to some functions #65

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

AntonReinhard
Copy link
Member

@AntonReinhard AntonReinhard commented Oct 6, 2024

This is dangerous and we should instead find the actual reason for the inconsistency. I'll leave this as a draft for now and will look into it more later. Since Base.@assume_effects requires Julia 1.8 this will fail unit tests anyways.

Having these functions infer :consistent correctly by the compiler can significantly help with optimization in the compiler, so eventually we should try to make sure that most of QEDcore functionality has as many effects inferred as possible.

Edit: I have removed the assumptions again because they are not safe to make (at least not for all element types). We should come back to this at a later point. For now, I have only added some @inbounds statements.

@AntonReinhard AntonReinhard requested a review from szabo137 October 6, 2024 10:47
@AntonReinhard
Copy link
Member Author

AntonReinhard commented Oct 8, 2024

Base.@assume_effects is a feature of Julia 1.10 and up (at least in the syntax I use it here), so this PR will probably just have to wait until we drop support for earlier Julia versions once 1.10 becomes the LTS version.

@AntonReinhard
Copy link
Member Author

This is stale for now and I'll come back to it at some other time. Currently this should not be merged since the assumptions aren't safe to make.

@AntonReinhard AntonReinhard force-pushed the assume-effects branch 2 times, most recently from 3c5041a to d373092 Compare October 29, 2024 09:35
@AntonReinhard
Copy link
Member Author

I have removed the unsafe assumptions for now and only added some @inbounds that should be safe.

@AntonReinhard AntonReinhard marked this pull request as ready for review October 29, 2024 09:36
@AntonReinhard AntonReinhard changed the title Add assume :consistent for base_state functions Add @inline to some functions Oct 29, 2024
@AntonReinhard AntonReinhard marked this pull request as draft October 31, 2024 11:21
@AntonReinhard AntonReinhard marked this pull request as ready for review October 31, 2024 12:04
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

LGFM.

@szabo137 szabo137 merged commit d89144f into QEDjl-project:dev Oct 31, 2024
4 checks passed
@AntonReinhard AntonReinhard deleted the assume-effects branch November 5, 2024 10: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.

2 participants