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 scattered push/pop operations #499

Merged
merged 3 commits into from
Jul 12, 2023
Merged

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Jun 21, 2023

Initial PR to gather some feedback on push/pop instructions. Closes #407.

There's a possible further improvement at the cost of simplicity: we could special-case pushing zero values (immediate bitmask all zeroes) to do something useful instead. For instance, it could be used to push/pop all user registers using a single operation. However I feel like that wouldn't be too useful.

@Dentosal Dentosal added the comp:FVM Component: FuelVM label Jun 21, 2023
@Dentosal Dentosal self-assigned this Jun 21, 2023
@vaivaswatha
Copy link

Is the cost of this instruction going to depend on how many registers were pushed?

LGTM.

@IGI-111
Copy link
Contributor

IGI-111 commented Jul 12, 2023

LGTM as well

@Dentosal
Copy link
Member Author

Is the cost of this instruction going to depend on how many registers were pushed?

No. It's cheap enough to execute anyway, the cost between 1 and 24 pushed registers is likely smaller than per-instruction overhead.

@Dentosal Dentosal enabled auto-merge (squash) July 12, 2023 12:19
@Dentosal Dentosal merged commit 6d93912 into master Jul 12, 2023
@Dentosal Dentosal deleted the dento/scattered-push-pop branch July 12, 2023 16:24
@Voxelot
Copy link
Member

Voxelot commented Aug 3, 2023

One thing I'm wondering about is how safe it is to assume that the stack will be properly cleared after an internal function call before popping the registers back. Otherwise there's a risk that we'd be popping arbitrary data into the registers if the stack wasn't restored to its original state while pushing the registers. While this is a sway implementation concern, let's say I write a sway function that has an asm block which manually mucks with the stack by performing a cfe, would sway be able to properly restore the stack to its original state before the registers are popped while returning from that fn? @vaivaswatha

@vaivaswatha
Copy link

One thing I'm wondering about is how safe it is to assume that the stack will be properly cleared after an internal function call before popping the registers back. Otherwise there's a risk that we'd be popping arbitrary data into the registers if the stack wasn't restored to its original state while pushing the registers. While this is a sway implementation concern, let's say I write a sway function that has an asm block which manually mucks with the stack by performing a cfe, would sway be able to properly restore the stack to its original state before the registers are popped while returning from that fn? @vaivaswatha

Isn't this a concern today as well? where we push the registers to the stack before a call and pop them afterwards, but just with separate instructions instead of this one single one.

I'd say it's hard for the compiler to do anything safely if ASM blocks are used unsafely.

@Voxelot
Copy link
Member

Voxelot commented Aug 3, 2023

One workaround I can think of is if the compiler also pushed $sp to the stack before a function call, and after returning it shrinks the stack back to that value automatically (if $sp != $original_sp) before any popping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:FVM Component: FuelVM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PUSH and POP instructions.
4 participants