Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Make saving and restoring float registers optional #33

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Feb 6, 2024

Closes #27

I once again tried it out on esp-wifi, but sadly it didn't seem to make much difference. I guess there is a bottleneck somewhere else for Xtensa.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 6, 2024

Code is fine and in general I agree with #27 but it's probably footgun material - whenever someone runs into it, it would be fun to debug that 🤔

What esp-idf does for context switching (and what we shouldn't do IMHO) is to disable the FPU and as soon as it gets used their exception handler will save the float regs and enable it - when returning they remember if they need to restore the float regs

Maybe we should at least disable the FPU while the interrupt handler runs? Still a bit more overhead than doing nothing but hopefully cheaper than saving / restoring all those (mostly unused) registers

t.b.h. I'm not sure if this will ever be an issue - just wanted to mention it before LGTMing it

My guess would be that spilling the registers is quite expensive - we could and probably should have some "fast path" interrupts in future (we could save some of the save / restore cost also for RISCV) but unfortunately for esp-wifi's task switching we definitely need all that. I also thought about making interrupts generally cheap and come up with a "new" idea for task switching .... I just don't have that new idea, yet

@MabezDev
Copy link
Member Author

MabezDev commented Feb 6, 2024

Maybe we should at least disable the FPU while the interrupt handler runs? Still a bit more overhead than doing nothing but hopefully cheaper than saving / restoring all those (mostly unused) registers

The idea here is that if they someone uses floats here without the FPU enabled they'll get a fault instead of silently doing bad things? Sounds good to me, thanks for that! I will look into that soon.

@MabezDev
Copy link
Member Author

MabezDev commented Feb 8, 2024

image

Thanks for the pointer, this is much better! Nice idea :).

@MabezDev MabezDev force-pushed the opt-in-float-context branch from 7bea925 to bdfc9fa Compare February 8, 2024 10:23
@MabezDev MabezDev requested a review from bjoernQ February 8, 2024 10:23
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - good idea

@MabezDev MabezDev merged commit d42af19 into master Feb 8, 2024
@MabezDev MabezDev deleted the opt-in-float-context branch February 8, 2024 12:09
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 12, 2024

FYI I checked how expensive spilling the registers is and it's around 53 cycles - it's not nothing but much less than what I expected

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature to not save/restore float registers
2 participants