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

Tweaks to maths routines #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

petermcs
Copy link
Contributor

@petermcs petermcs commented May 6, 2023

Mostly R6K improvements to speed things up by using new instructions and careful alignment. Some instructions changed from 1 byte instruction to two byte equivelant to maximise the run of aligned instructions which improves things for 16 bit memory. Modified division to bail out early in 8 bit steps to reduce number of loops required. Added unsigned 64 by 32 division and lower overhead shifts to support porting of Mbed TLS

Mostly R6K improvements to speed things up by using new instructions and careful alignment.
Some instructions changed from 1 byte instruction to two byte equivelant to maximise the run of aligned instructions which improves things for 16 bit memory.
Modified division to bail out early in 8 bit steps to reduce number of  loops required.
Added unsigned 64 by 32 division and  lower overhead shifts to support porting of Mbed TLS
@tomlogic
Copy link
Contributor

tomlogic commented May 8, 2023

Did you write any test programs to validate this code, with coverage for multiple cases? If you've been using it extensively in your software and updated TLS code, it's probably safe to merge. But there would be some benefit to ensuring edge cases still give the expected answer.

If you have test code, can you add it as sample programs? I'm even open to having a Samples/Test directory with test programs to validate various functions.

align even
_fast_asr::
rr a
jr nc,@pc+5
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize the original code used the @pc+x notation, but seeing it here feels risky -- it depends on knowing exactly how many opcodes show up in following statements. Is there a reason we can't just replace the jr destinations with local labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced in the new code and the old code. There is one instance left in a macro but there is no easy way to handle those

@@ -93,7 +93,7 @@ __root void L_div();
; BCDE = Dividend / Divisor
; HL'HL = Dividend % Divisor
; Modulus is given same sign as dividend (numerator).

align even
Copy link
Contributor

Choose a reason for hiding this comment

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

As in my other reviews, I'd like to see at least a short comment on these align statements that we're targeting even alignment for improved performance on 5000/6000 with 16-bit memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@petermcs
Copy link
Contributor Author

petermcs commented May 8, 2023

Did you write any test programs to validate this code, with coverage for multiple cases? If you've been using it extensively in your software and updated TLS code, it's probably safe to merge. But there would be some benefit to ensuring edge cases still give the expected answer.

If you have test code, can you add it as sample programs? I'm even open to having a Samples/Test directory with test programs to validate various functions.

I did have some test code when I was doing the development but didn't hold on to it. It has been used extensively and the Mbed TLS test code which I've run a lot is very sensitive to any errors...

Removed as many of the pc+n jumps as possible and replaced them with jumps to local labels
@tomlogic
Copy link
Contributor

I'm finally getting around to reviewing your PRs and merging them. Do you have any stats on the improvement you saw with the new code? Or was the change primarily to support the addition of G_div_ll_l()?

@petermcs
Copy link
Contributor Author

I'm finally getting around to reviewing your PRs and merging them. Do you have any stats on the improvement you saw with the new code? Or was the change primarily to support the addition of G_div_ll_l()?

Thanks for that Tom.

I don't have any stats but from memory they were small improvements and would have shaved a few milliseconds of some of the mbedtls test run times - nothing dramatic but with the amount of processing the TLS stuff requires, every little bit helps! Since I started the mbedtls port I've managed to improve the test run times to the point where they are a quarter of the original but when one test case takes 27 seconds to run in the first place, getting it down to 7 seconds still leaves a bit to be desired...

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