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

Feature - Swaps parameters of sub reg, imm and removes neg #489

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Jul 26, 2023

Currently the instruction sub reg, imm is useless because it can be encoded as add reg, -imm instead.
If we swap the parameters of sub reg, imm from reg = reg - imm to reg = imm - reg it becomes a useful instruction which would otherwise require two instructions to encode. Furthermore, the neg instruction can be removed as well because it can be encoded as sub reg, 0 (reg = 0 - reg) instead.

It seems that llvm is never emitting sub reg, imm for registers other than R11 (which was already addressed in #488).
For neg the instruction info in llvm would need to be patched here:
https://github.com/solana-labs/llvm-project/blob/7b8db05b564faffb522434b73b7082662171f94a/llvm/lib/Target/SBF/SBFInstrInfo.td#L319

@Lichtso Lichtso requested a review from alessandrod July 26, 2023 12:22
@codecov-commenter
Copy link

Codecov Report

Merging #489 (6bc459c) into main (5debf64) will decrease coverage by 0.05%.
The diff coverage is 76.00%.

@@            Coverage Diff             @@
##             main     #489      +/-   ##
==========================================
- Coverage   89.69%   89.65%   -0.05%     
==========================================
  Files          23       23              
  Lines       10109    10126      +17     
==========================================
+ Hits         9067     9078      +11     
- Misses       1042     1048       +6     
Files Changed Coverage Δ
src/ebpf.rs 79.59% <ø> (ø)
src/interpreter.rs 97.66% <66.66%> (-0.57%) ⬇️
src/jit.rs 92.63% <71.42%> (-0.27%) ⬇️
src/elf.rs 87.34% <100.00%> (+0.02%) ⬆️
src/verifier.rs 97.45% <100.00%> (ø)

@Lichtso Lichtso mentioned this pull request Jul 26, 2023
18 tasks
@@ -264,7 +264,11 @@ impl<'a, 'b, V: Verifier, C: ContextObject> Interpreter<'a, 'b, V, C> {
// BPF_ALU class
ebpf::ADD32_IMM => self.reg[dst] = (self.reg[dst] as i32).wrapping_add(insn.imm as i32) as u64,
ebpf::ADD32_REG => self.reg[dst] = (self.reg[dst] as i32).wrapping_add(self.reg[src] as i32) as u64,
ebpf::SUB32_IMM => self.reg[dst] = (self.reg[dst] as i32).wrapping_sub(insn.imm as i32) as u64,
ebpf::SUB32_IMM => if self.executable.get_sbpf_version().enable_neg() {
Copy link

@alessandrod alessandrod Jul 28, 2023

Choose a reason for hiding this comment

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

I would use a more explicit swap_sub_operands() or something instead of
piggybacking on enable_neg. These flags are cheap and having an explicit one
makes the logic easier to follow.

And I would also add an if enable_neg() to the ::NEG_* handlers again for
explicitness, and so it doesn't have to rely on the verifier doing things at a distance

@Lichtso Lichtso force-pushed the feature/merge_neg_and_sub_imm branch from 6bc459c to 57ece7c Compare July 28, 2023 16:29
@Lichtso Lichtso merged commit 2fe4357 into main Jul 28, 2023
12 checks passed
@Lichtso Lichtso deleted the feature/merge_neg_and_sub_imm branch July 28, 2023 16:51
@Lichtso Lichtso changed the title Feature - Swaps parameters of "sub imm" and removes "neg" Feature - Swaps parameters of sub reg, imm and removes neg Aug 24, 2023
nvjle pushed a commit to anza-xyz/llvm-project that referenced this pull request Dec 16, 2023
Following the alterations made in solana-labs/rbpf#489, this PR removes the neg instructions and changes the semantics of sub when one of the operands is an immediate.

sub r1, 2 now means r1 = 2 - r1, instead of r1 = r1 - 2.
neg r1 is represented as r1 = 0  - r1.

This is the second task in solana-labs/solana#34250.
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jan 31, 2024
Following the alterations made in solana-labs/rbpf#489, this PR removes the neg instructions and changes the semantics of sub when one of the operands is an immediate.

sub r1, 2 now means r1 = 2 - r1, instead of r1 = r1 - 2.
neg r1 is represented as r1 = 0  - r1.

This is the second task in solana-labs/solana#34250.
LucasSte added a commit to anza-xyz/llvm-project that referenced this pull request Feb 16, 2024
Following the alterations made in solana-labs/rbpf#489, this PR removes the neg instructions and changes the semantics of sub when one of the operands is an immediate.

sub r1, 2 now means r1 = 2 - r1, instead of r1 = r1 - 2.
neg r1 is represented as r1 = 0  - r1.

This is the second task in solana-labs/solana#34250.
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jun 28, 2024
Following the alterations made in solana-labs/rbpf#489, this PR removes the neg instructions and changes the semantics of sub when one of the operands is an immediate.

sub r1, 2 now means r1 = 2 - r1, instead of r1 = r1 - 2.
neg r1 is represented as r1 = 0  - r1.

This is the second 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.

None yet

3 participants