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

Create PQR instruction class #81

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

LucasSte
Copy link
Collaborator

This PR creates the PQR instruction class for SBFv2, as it introduced in solana-labs/rbpf#498.
This is another task in solana-labs/solana#34250 to implement the SBFv2 changes in the LLVM backend.

I also tested these new instructions with all the programs in programs/sbf on the main repository. The only failure I got was a different instruction count, so I assume everything is working.

@LucasSte LucasSte marked this pull request as ready for review January 17, 2024 12:39
@seanyoung
Copy link

I think it would be nice to have a test for 256 bit mul, to ensure the codegen is good. Possibly with overflow detection too.

This change makes a big difference for NeonVM and Solang. It would nice to ensure it works as expected.

@LucasSte
Copy link
Collaborator Author

I think it would be nice to have a test for 256 bit mul, to ensure the codegen is good. Possibly with overflow detection too.

This change makes a big difference for NeonVM and Solang. It would nice to ensure it works as expected.

What do you mean by "codegen is good"? 256-bit multiplications generate many assembly instructions.

@seanyoung
Copy link

I think it would be nice to have a test for 256 bit mul, to ensure the codegen is good. Possibly with overflow detection too.
This change makes a big difference for NeonVM and Solang. It would nice to ensure it works as expected.

What do you mean by "codegen is good"? 256-bit multiplications generate many assembly instructions.

If the mulhu and mulhs instructions are used, it should be much shorter (about 30 instructions). A test would verify that those instructions are indeed used (and used correctly, of course).

@LucasSte
Copy link
Collaborator Author

LucasSte commented Jan 17, 2024

I think it would be nice to have a test for 256 bit mul, to ensure the codegen is good. Possibly with overflow detection too.
This change makes a big difference for NeonVM and Solang. It would nice to ensure it works as expected.

What do you mean by "codegen is good"? 256-bit multiplications generate many assembly instructions.

If the mulhu and mulhs instructions are used, it should be much shorter (about 30 instructions). A test would verify that those instructions are indeed used (and used correctly, of course).

I'll include a test for that. Keep in mind that correctly breaking down 256-bit operations happens in LLVM's target independent code generation. This PR only matches the ISDOpCode MULHU/MULHS with the new instructions if ISDOpCodes are generated. Emitting mulhu or not for such a case is not something this PR addresses, as it is not related to the SBF backend.

The function below has more than 100 instructions and utilizes uhmul64 six times between many conditional jumps.

define i256 @try_mul(i256 noundef %a, i256 noundef %b) local_unnamed_addr #0 {
entry:
  %mul = mul i256 %a, %b
  ret i256 %mul
}

@seanyoung
Copy link

The function below has more than 100 instructions and utilizes uhmul64 six times between many conditional jumps.

define i256 @try_mul(i256 noundef %a, i256 noundef %b) local_unnamed_addr #0 {
entry:
  %mul = mul i256 %a, %b
  ret i256 %mul
}

That's broken then

@LucasSte
Copy link
Collaborator Author

The function below has more than 100 instructions and utilizes uhmul64 six times between many conditional jumps.

define i256 @try_mul(i256 noundef %a, i256 noundef %b) local_unnamed_addr #0 {
entry:
  %mul = mul i256 %a, %b
  ret i256 %mul
}

That's broken then

Ok, I'm not adding the test then. I counted 16 multiplications in total (10 lmul64 and 6 uhmu64). Maybe the algorithm is not as lean as we were expecting.

Copy link
Collaborator

@dmakarov dmakarov left a comment

Choose a reason for hiding this comment

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

LGTM.

@LucasSte LucasSte merged commit be616a0 into anza-xyz:solana-rustc/16.0-2023-06-05 Jan 18, 2024
14 checks passed
@LucasSte LucasSte deleted the pqr-p2n branch January 18, 2024 13:58
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jan 31, 2024
LucasSte added a commit that referenced this pull request Feb 16, 2024
* Create PQR instruction class
LucasSte added a commit to LucasSte/llvm-project that referenced this pull request Jun 28, 2024
LucasSte added a commit that referenced this pull request Aug 19, 2024
* Create PQR instruction class
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.

3 participants