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

Refactor - Dynamic Stack #638

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Refactor - Dynamic Stack #638

merged 7 commits into from
Nov 27, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Nov 27, 2024

R11 is removed and instead R10 is modified directly and its changes visible immediately. This means that R10 is always the lower end of the currently executing functions stack frame and all stack accesses must be encoded relative to that.

Additionally, the verifier allows add64 r10, imm and checks that the imm is 64 byte aligned.

@Lichtso Lichtso requested a review from LucasSte November 27, 2024 12:43
Copy link

@LucasSte LucasSte left a comment

Choose a reason for hiding this comment

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

One of the CI tests is failing.


### from v1
- `add64 reg, imm` can use `r11` as destination register
- `add64 reg, imm` can also use `r10` as destination register

Choose a reason for hiding this comment

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

Perhaps a better wording here is "Only add64 reg, imm can use r10 as a destination register".

Copy link
Author

Choose a reason for hiding this comment

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

But stores can still use R10 as dst, right?

Choose a reason for hiding this comment

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

Maybe "Only add64 reg, imm can modify R10"?

Copy link
Author

Choose a reason for hiding this comment

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

The semantics are in execution, the verifier can only decide what dst regs are allowed.

Comment on lines 31 to 32
/// Frame pointer register
pub const FRAME_PTR_REG: usize = 10;

Choose a reason for hiding this comment

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

Should we start calling it stack pointer for the new version?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe, but different PR. Also, v0 will continue to exist.

call function_foo
mov r0, r10
exit
function_foo:
add r10, -64

Choose a reason for hiding this comment

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

Can we test stores and loads with positive offsets from R10? Just like test_string_stack.

@Lichtso Lichtso force-pushed the refactor/dynamic_stack branch from 7a20be2 to 9afb4cf Compare November 27, 2024 16:21
@Lichtso Lichtso force-pushed the refactor/dynamic_stack branch from 9afb4cf to d517ed2 Compare November 27, 2024 17:46
@Lichtso Lichtso merged commit 4ad935b into main Nov 27, 2024
12 checks passed
@Lichtso Lichtso deleted the refactor/dynamic_stack branch November 27, 2024 22:46
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