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

irq: Optimize interrupt save/restore #104

Merged
merged 1 commit into from
Sep 7, 2022
Merged

irq: Optimize interrupt save/restore #104

merged 1 commit into from
Sep 7, 2022

Conversation

mbuesch
Copy link
Contributor

@mbuesch mbuesch commented Aug 5, 2022

Avoid unnecessary mask and branch instructions.

The basic reasoning behind this is that all other flags in the SREG can be clobbered without ill effects. The restore() function is an optimization fence and the compiler is not allowed to make assumptions about memory or SREG state after execution.

This avoids an and and a breq instruction or similar in every critical section.

This is an API breakage that requires an incompatible version bump.
Question: Should we return a struct instead of u8? That would give us the opportunity to change semantics without breaking API in the future.

I hope I got the llvm_asm! part of restore() right. I didn't test it. What about removing all llvm_asm!?

Rahix
Rahix reviewed Aug 8, 2022
View reviewed changes
Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I like the change, this definitely goes in the right direction. See comments below.

src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
@mbuesch mbuesch mentioned this pull request Aug 8, 2022
Merged
@mbuesch mbuesch force-pushed the irqopt branch 4 times, most recently from c73c856 to 99c4605 Compare August 8, 2022 19:28
@mbuesch
Copy link
Contributor Author

mbuesch commented Aug 8, 2022

I resolved all review findings. I'll rebase this again after #106 has been resolved an pulled.

@mbuesch mbuesch force-pushed the irqopt branch 2 times, most recently from fe5e137 to d9fdc24 Compare August 9, 2022 13:29
@mbuesch
Copy link
Contributor Author

mbuesch commented Aug 9, 2022

Rebased on top of latest main and removed a couple of compiler warnings in non-avr mode.

@mbuesch mbuesch mentioned this pull request Aug 9, 2022
Merged
Rahix
Rahix requested changes Sep 5, 2022
View reviewed changes
Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Hey, sorry for letting this wait for so long... Thanks again for the changeset, this change is great and I really like your latest version. I have a few comments left, see below. Again, sorry for the delay.

Cargo.toml Outdated
@@ -62,8 +62,13 @@ docsrs = ["rt", "atmega328p", "atmega32u4", "atmega2560", "attiny85", "atmega480
bare-metal = "0.2.5"
vcell = "0.1.2"
cfg-if = "0.1.10"
ufmt = "0.1.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should make ufmt an optional dependency.

Suggested change
ufmt = "0.1.0"
ufmt = { version = "0.1.0", optional = true }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made ufmt optional.
I called the feature udebug, because that's really what it enables. Makes it more transparent to the user what's being enabled.

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about the feature name... But I'll check what others in the ecosystem are doing and will adjust accordingly after merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finding good names. The biggest problem in software development :)
Thanks for merging.

Cargo.toml Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
src/interrupt.rs Outdated Show resolved Hide resolved
@mbuesch
irq: Optimize interrupt save/restore
9825a6a 
Avoid unnecessary mask and branch instructions
@mbuesch
Copy link
Contributor Author

mbuesch commented Sep 6, 2022

Thanks again for your review.
I addressed all your points.

Rahix
Rahix approved these changes Sep 7, 2022
View reviewed changes
Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Hey, thank you so much!

@Rahix Rahix merged commit 280d685 into Rahix:main Sep 7, 2022
mbuesch added a commit to mbuesch/avr-device that referenced this pull request Sep 7, 2022
@mbuesch
Cargo.toml: Update edition to 2021
e2e1bc4 
We have recently (Rahix#104) dropped support for compilers that don't support edition 2021.
@Rahix
Copy link
Owner

Rahix commented Sep 8, 2022

Got the notification for your commit above this comment. I want to break a new release soon, with all your changes, but then I'll wait a bit, still?

mbuesch added a commit to mbuesch/avr-device that referenced this pull request Sep 8, 2022
@mbuesch
Cargo.toml: Update edition to 2021
3b3904e 
We have recently (Rahix#104) dropped support for compilers that don't support edition 2021.
@mbuesch mbuesch mentioned this pull request Sep 8, 2022
Merged
@mbuesch
Copy link
Contributor Author

mbuesch commented Sep 8, 2022

Oh, I think any name is fine. Please pick the name you feel is best. Be it udebug, ufmt or anything else.
I agree that we should have a release asap. But, maybe we should include #109 in that release, too?
With that applied I think this is ready for another release.

Rahix pushed a commit that referenced this pull request Sep 8, 2022
@mbuesch
Cargo.toml: Update edition to 2021
6a9771e 
We have recently (#104) dropped support for compilers that don't support
edition 2021. Make this explicit by enforcing edition 2021 in
Cargo.toml.
@Rahix
Copy link
Owner

Rahix commented Sep 8, 2022

Here you go: https://github.com/Rahix/avr-device/releases/tag/v0.4.0 :)

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

@Rahix Rahix Rahix approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@mbuesch @Rahix