Skip to content

Optimize code format, instruction order and flag usage in RV64 Montgomery multiplication (RISC-V) - #29438

Closed
fengpengboa wants to merge 2 commits into
openssl:masterfrom
fengpengboa:en_rv64-mont-asm
Closed

Optimize code format, instruction order and flag usage in RV64 Montgomery multiplication (RISC-V)#29438
fengpengboa wants to merge 2 commits into
openssl:masterfrom
fengpengboa:en_rv64-mont-asm

Conversation

@fengpengboa

@fengpengboa fengpengboa commented Dec 18, 2025

Copy link
Copy Markdown
Contributor

This PR performs further optimization based on the already merged "Implement Montgomery multiplication assembly optimization for RV64GC" (#28012).
The key improvements include:code formatting unification、instruction scheduling optimization、register allocation strategy update、updates to addition/subtraction carry and borrow flag handling.
These enhancements aim to improve both the code maintainability and its execution performance, particularly on out-of-order RISC-V cores.

Performance Improvement

openssl speed  -seconds 1 rsa2048

benchmark results run on SG2042:

Test Item Baseline (C) ASM Optimized Improvement
​sign/s 208 213.0 2.4%
​verify/s 7190 7415.2 3.13%
encr./s 6638 6814.1 2.65%
decr./s 203 209.0 2.96%

@github-actions github-actions Bot added the severity: fips change The pull request changes FIPS provider sources label Dec 18, 2025
@loci-dev loci-dev mentioned this pull request Dec 18, 2025
Open
@fengpengboa

fengpengboa commented Dec 18, 2025

Copy link
Copy Markdown
Contributor Author

Hi @t8m @paulidale @nhorman @ZenithalHourlyRate @HeliC829 @JerryShih !! My submitted PR is currently in Open status. When convenient, could the maintainer team please help move the review forward? I'm happy to address any needed changes or provide clarifications.Thank you for your time and consideration!

nhorman
nhorman requested changes Dec 18, 2025
View reviewed changes

@nhorman nhorman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Theres nothing to egregious in here, but it seems like there are a few alignment issues. I'm not sure if this applies here, but are you running clang-format on this perl script? If so, we explicitly exclude any non-c files from formatting, specifically because clang-format isn't meant to format perl.

Comment thread crypto/bn/asm/riscv64-mont.pl Outdated
$ret.=" sd $_,$stack_offset(sp)\n";
}
return $ret;
return $ret;

@nhorman nhorman Dec 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't seem properly aligned

Comment thread crypto/bn/asm/riscv64-mont.pl Outdated
$ret.=" ld $_,$stack_offset(sp)\n";
}
$ret.=" addi sp,sp,$stack_reservation\n";
$ret.=" addi sp,sp,$stack_reservation\n";

@nhorman nhorman Dec 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nor does this.

@fengpengboa

fengpengboa commented Dec 19, 2025

Copy link
Copy Markdown
Contributor Author

Theres nothing to egregious in here, but it seems like there are a few alignment issues. I'm not sure if this applies here, but are you running clang-format on this perl script? If so, we explicitly exclude any non-c files from formatting, specifically because clang-format isn't meant to format perl.

Thank you for catching this. I'll correct the formatting issues manually.

@fengpengboa
This PR performs further optimization based on the already merged "Im…
16a89f9 
…plement Montgomery multiplication assembly optimization for RV64GC" (openssl#28012).

The key improvements include:code formatting unification、instruction scheduling optimization、register allocation strategy update、updates to addition/subtraction carry and borrow flag handling.
These enhancements aim to improve both the code maintainability and its execution performance, particularly on out-of-order RISC-V cores.
@fengpengboa

fengpengboa commented Dec 19, 2025

Copy link
Copy Markdown
Contributor Author

Theres nothing to egregious in here, but it seems like there are a few alignment issues. I'm not sure if this applies here, but are you running clang-format on this perl script? If so, we explicitly exclude any non-c files from formatting, specifically because clang-format isn't meant to format perl.

Hello @nhorman! I have now addressed all your previous review comments. The latest changes have been pushed.

nhorman
nhorman previously approved these changes Dec 19, 2025
View reviewed changes
@fengpengboa

fengpengboa commented Dec 20, 2025

Copy link
Copy Markdown
Contributor Author

Hi @paulidale , could you please review this PR when you have a moment? Thank you!

ZenithalHourlyRate
ZenithalHourlyRate reviewed Dec 21, 2025
View reviewed changes

@ZenithalHourlyRate ZenithalHourlyRate left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

paulidale
paulidale previously approved these changes Dec 21, 2025
View reviewed changes
Comment thread crypto/bn/asm/riscv64-mont.pl Resolved
@paulidale paulidale added approval: done This pull request has the required number of approvals branch: master Applies to master branch tests: exempted The PR is exempt from requirements for testing triaged: performance The issue/pr reports/fixes a performance concern labels Dec 21, 2025
@paulidale paulidale changed the title Optimize code format, instruction order and flag usage in RV64 Montgomery multiplication Optimize code format, instruction order and flag usage in RV64 Montgomery multiplication (RISC-V) Dec 21, 2025
@fengpengboa
fixup! This PR performs further optimization based on the already mer…
169e4d0 
…ged "Implement Montgomery multiplication assembly optimization for RV64GC" (openssl#28012). The key improvements include:code formatting unification、instruction scheduling optimization、register allocation strategy update、updates to addition/subtraction carry and borrow flag handling. These enhancements aim to improve both the code maintainability and its execution performance, particularly on out-of-order RISC-V cores.
@fengpengboa
fengpengboa dismissed stale reviews from paulidale and nhorman via 169e4d0 December 22, 2025 02:58
@paulidale

paulidale commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

No need for a timer reset.

@paulidale
paulidale requested a review from nhorman December 22, 2025 03:02
@fengpengboa

fengpengboa commented Dec 22, 2025

Copy link
Copy Markdown
Contributor Author

@nhorman Hi,I've added the missing newline using a fixup commit. The PR has been updated and is ready for your final review when you have a moment.

HeliC829
HeliC829 reviewed Dec 22, 2025
View reviewed changes

@HeliC829 HeliC829 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@openssl-machine

openssl-machine commented Dec 23, 2025

Copy link
Copy Markdown
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@fengpengboa

fengpengboa commented Dec 24, 2025

Copy link
Copy Markdown
Contributor Author

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Hi ,@nhorman @paulidale, The PR was updated after receiving approval: done, so the automatic label wasn’t applied.Since I don’t have the required permissions, could someone with access please add the approval: ready to merge label?Appreciate your help.

@nhorman nhorman removed the approval: done This pull request has the required number of approvals label Dec 30, 2025
@nhorman nhorman added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Dec 30, 2025
@nhorman

nhorman commented Dec 30, 2025

Copy link
Copy Markdown
Contributor

merged to master, thank you

@nhorman nhorman closed this Dec 30, 2025
openssl-machine pushed a commit that referenced this pull request Dec 30, 2025
@fengpengboa @nhorman
This PR performs further optimization based on the already merged "Im…
b34e890 
…plement Montgomery multiplication assembly optimization for RV64GC" (#28012).

The key improvements include:code formatting unification、instruction scheduling optimization、register allocation strategy update、updates to addition/subtraction carry and borrow flag handling.
These enhancements aim to improve both the code maintainability and its execution performance, particularly on out-of-order RISC-V cores.

Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #29438)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nhorman nhorman nhorman approved these changes
@paulidale paulidale paulidale approved these changes
+2 more reviewers
@HeliC829 HeliC829 HeliC829 left review comments
@ZenithalHourlyRate ZenithalHourlyRate ZenithalHourlyRate left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: performance The issue/pr reports/fixes a performance concern

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fengpengboa @paulidale @openssl-machine @nhorman @HeliC829 @ZenithalHourlyRate