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] Remove neg and change the semantics of sub #73

Merged
merged 4 commits into from
Dec 16, 2023

Conversation

LucasSte
Copy link
Collaborator

@LucasSte LucasSte commented Dec 12, 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 LucasSte changed the title Remove neg and change the semantics of sub Remove neg and change the semantics of sub Dec 12, 2023
@LucasSte LucasSte changed the title Remove neg and change the semantics of sub [SOL] Remove neg and change the semantics of sub Dec 12, 2023
@LucasSte LucasSte marked this pull request as ready for review December 13, 2023 13:33
@nvjle nvjle self-requested a review December 13, 2023 14:41
@@ -306,24 +331,6 @@ let Constraints = "$dst = $src2" in {
}
}
Copy link

Choose a reason for hiding this comment

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

The entire part of the patch above involving the ALU multiclass and the SUB definition need to be reworked and simplified. That is, there is no need to refactor and duplicate the multiclass. We can succinctly adjust the original multiclass by conditionally swapping the pattern operands as follows:

@@ -264,6 +264,12 @@ class ALU_RR<SBFOpClass Class, SBFArithOp Opc,
 }
 
 multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
+  // Match swapped operand order only for special case of 'sub reg,imm`.
+  defvar DoSwap = !eq(OpNode, sub);
+  defvar RegImmPat = !if(DoSwap, (OpNode i64immSExt32:$imm, GPR:$src2),
+                                 (OpNode GPR:$src2, i64immSExt32:$imm));
+  defvar RegImmPat32 = !if(DoSwap,(OpNode i32immSExt32:$imm, GPR32:$src2),
+                                  (OpNode GPR32:$src2, i32immSExt32:$imm));
   def _rr : ALU_RR<SBF_ALU64, Opc,
                    (outs GPR:$dst),
                    (ins GPR:$src2, GPR:$src),
@@ -273,7 +279,7 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNod
e> {
                    (outs GPR:$dst),
                    (ins GPR:$src2, i64imm:$imm),
                    Mnemonic # "64 $dst, $imm",
-                   [(set GPR:$dst, (OpNode GPR:$src2, i64immSExt32:$imm))]>;
+                   [(set GPR:$dst, RegImmPat)]>;
   def _rr_32 : ALU_RR<SBF_ALU, Opc,
                    (outs GPR32:$dst),
                    (ins GPR32:$src2, GPR32:$src),
@@ -283,7 +289,7 @@ multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNod
e> {
                    (outs GPR32:$dst),
                    (ins GPR32:$src2, i32imm:$imm),
                    Mnemonic # "32 $dst, $imm",
-                   [(set GPR32:$dst, (OpNode GPR32:$src2, i32immSExt32:$imm))]>
;
+                   [(set GPR32:$dst, RegImmPat32)]>;
 }
 
 let Constraints = "$dst = $src2" in {
@@ -306,24 +312,6 @@ let Constraints = "$dst = $src2" in {
     }
 }

Copy link

Choose a reason for hiding this comment

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

One related general question I have is-- should we conditionally be generating the swapped special case sub? We need to exactly match the runtime semantics on chain today and down the road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new syntax can be behind the sbfv2 cpu perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't find a way to encapsulate the new sub inside a predicate for sbfv2. All the other example cases do not redefine the same instruction. Instead, the predicate encapsulate definitions that only exist for a certain feature and do not override other definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Everything should be behind a feature flag now.

Copy link

Choose a reason for hiding this comment

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

Thanks for incorporating my offline suggestions, this is ready to land (other than some trivial coding standard nits, which I'll note shortly).

@LucasSte LucasSte force-pushed the neg-sub branch 2 times, most recently from 5cd9dee to 6185be4 Compare December 14, 2023 18:16
Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>
@LucasSte LucasSte force-pushed the neg-sub branch 2 times, most recently from abdbb6e to 5326886 Compare December 14, 2023 22:30
@LucasSte LucasSte requested a review from nvjle December 14, 2023 22:40
@@ -263,7 +265,7 @@ class ALU_RR<SBFOpClass Class, SBFArithOp Opc,
let SBFClass = Class;
}

multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode> {
multiclass ALU<SBFArithOp Opc, string Mnemonic, SDNode OpNode, bit UseImmPat = 1> {
Copy link

Choose a reason for hiding this comment

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

LLVM Coding standard nit: (80-column rules) Please move the new parameter to the next line, aligned with the <'.

@@ -306,6 +312,19 @@ let Constraints = "$dst = $src2" in {
}
}

// Special case for SBFv2
// In SBFv1, `sub reg, imm` is interpreted as reg = reg - imm,
// but in SBFv2 it means reg = imm - reg
Copy link

Choose a reason for hiding this comment

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

LLVM Coding standard nit: (Proper prose, capitalization, punctuation) Please add period/full-stops (as in the original comments that I had written-- but your wording is fine).

+// Special case cpu wart:
+// Operand order for 'sub reg,imm' is interpreted
+// differently for pre-sbfv2 cpu versus sbfv2. In the former case, the
+// VM executes 'reg-imm'. For the latter, 'imm-reg'.
+// Define two sets of 'sub' patterns accordingly.

}

// Instruction `neg` exists on SBFv1, but not on SBFv2
// In SBFv2, the negate operation is done with a subtraction
Copy link

Choose a reason for hiding this comment

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

Ditto: Periods/full-stops.

@nvjle nvjle merged commit 17edb2d into anza-xyz:solana-rustc/16.0-2023-06-05 Dec 16, 2023
14 checks passed
@LucasSte LucasSte deleted the neg-sub branch December 18, 2023 12:25
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 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

2 participants