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

SIMD-0166: SBPF Dynamic stack frames #166

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

LucasSte
Copy link

No description provided.

@LucasSte LucasSte changed the title SIMD-XXX: Dynamic stack frames SIMD-0166: Dynamic stack frames Aug 19, 2024
@LucasSte LucasSte changed the title SIMD-0166: Dynamic stack frames SIMD-0166: SBPF Dynamic stack frames Oct 2, 2024
proposals/0166-dynamic-stack-frames.md Outdated Show resolved Hide resolved
proposals/0166-dynamic-stack-frames.md Outdated Show resolved Hide resolved
Copy link
Contributor

@0x0ece 0x0ece left a comment

Choose a reason for hiding this comment

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

This looks a huge improvement. My only recommendation is alignment, I tried to explain why I'd push it up to 64 bytes vs just 16.

proposals/0166-dynamic-stack-frames.md Outdated Show resolved Hide resolved
call convention obviates the need to use R5 for retrieving the caller’s frame
pointer address to access those parameters.

### Changes in the verifier
Copy link
Contributor

Choose a reason for hiding this comment

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

I would simplify this section. ATM it seems to describe the whole process of validating registers, not just the changes due to this SIMD.

The verifier should include a new rule to accept R11 as a destination register under the following conditions:

  1. Dynamic stack frames are enabled
  2. The corresponding instruction is add64_imm (opcode 0x07)
  3. The corresponding value of imm is a multiple of the alignment (i.e. multiple of 64).

Copy link
Contributor

Choose a reason for hiding this comment

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

As a comment to my comment :) Based on the current code, I'd simply add an extra "and" to the "if" condition that returns Ok() for R11. Therefore I don't think we need an error for alignment. Either R11 is allowed (if all 3 conditions pass), or it's not.

Copy link
Author

Choose a reason for hiding this comment

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

I re-wrote the section, based on the new design we devised with only R10.

proposals/0166-dynamic-stack-frames.md Show resolved Hide resolved
proposals/0166-dynamic-stack-frames.md Outdated Show resolved Hide resolved
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.

5 participants