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

[SOL] Use ADD for subtracting stack pointer #70

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

LucasSte
Copy link
Collaborator

@LucasSte LucasSte commented Dec 8, 2023

This PR replaces sub r11, imm by add r11, -imm and is the equivalent of solana-labs/rbpf#488 for the SBF target in LLVM.

This is the first task in solana-labs/solana#34250

@LucasSte LucasSte changed the title Use ADD for subtracting stack pointer [SOL] Use ADD for subtracting stack pointer Dec 8, 2023
@nvjle
Copy link

nvjle commented Dec 10, 2023

The patch LGTM (see minor nits mentioned). Note, however, that the MacOS CI failed due to some Mac flakiness (unrelated to your patch). You will need to first rebase to get a minor patch I just committed to clean that up.

@nvjle nvjle self-requested a review December 10, 2023 03:40
llvm/lib/Target/SBF/SBFFrameLowering.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/SBF/SBFFrameLowering.cpp Outdated Show resolved Hide resolved
.addReg(SBF::R11)
.addImm(NumBytes);
.addImm(IsSubtract? -NumBytes : NumBytes);
Copy link

Choose a reason for hiding this comment

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

Reminder: please be mindful of the LLVM coding conventions[*]. You can run clang-format to help identify/automate correct coding conventions. Upstream LLVM runs this automatically for submitted patches, but we do not yet do this in our local CI.
Also, when you run clang-format on future patches, only commit the diffs that are within the lines of your patch.

[*] See the links in, e.g., https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I keep a single commit in the PR?

Copy link

Choose a reason for hiding this comment

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

Yes, you should do a squash commit (with updated log message for the full commit) when done. The github UI will ask you.

@LucasSte LucasSte marked this pull request as ready for review December 11, 2023 22:28
@LucasSte LucasSte requested a review from nvjle December 11, 2023 22:28
Copy link

@nvjle nvjle left a comment

Choose a reason for hiding this comment

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

LGTM, ready to land.

@nvjle nvjle merged commit 3bddeb1 into anza-xyz:solana-rustc/16.0-2023-06-05 Dec 12, 2023
14 checks passed
@LucasSte LucasSte deleted the stack-frame branch December 13, 2023 17:07
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jan 31, 2024
This PR replaces sub r11, imm by add r11, -imm and is the equivalent of solana-labs/rbpf#488 for the SBF target in LLVM.

This is the first task in solana-labs/solana#34250
LucasSte added a commit that referenced this pull request Feb 16, 2024
This PR replaces sub r11, imm by add r11, -imm and is the equivalent of solana-labs/rbpf#488 for the SBF target in LLVM.

This is the first task in solana-labs/solana#34250
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jun 28, 2024
This PR replaces sub r11, imm by add r11, -imm and is the equivalent of solana-labs/rbpf#488 for the SBF target in LLVM.

This is the first task in solana-labs/solana#34250
LucasSte added a commit that referenced this pull request Aug 19, 2024
This PR replaces sub r11, imm by add r11, -imm and is the equivalent of solana-labs/rbpf#488 for the SBF target in LLVM.

This is the first task in solana-labs/solana#34250
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