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

Enhance the precision of fixed-point square root computations #16

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

marvin0102
Copy link
Collaborator

@marvin0102 marvin0102 commented Jul 24, 2024

The original fixed point sqrt can not calculate sqrt of numbers less than 1. Implement a new sqrt calculation that is more precise and without fixed point multiplication.
Below is the comparison of sqrt from 0~2. "math.h" stands for function 'sqrt()' from library math.h. "original" stands for original sqrt implementation. "precise" stands for new implemented method.
sqrt_compare

jserv

This comment was marked as outdated.

@jserv jserv changed the title Implement precise square root calculation for fixed point Enhance the precision of fixed-point square root computations Jul 24, 2024
@jserv jserv requested a review from weihsinyeh July 24, 2024 13:28
src/fixed.c Outdated Show resolved Hide resolved
@marvin0102 marvin0102 force-pushed the main branch 3 times, most recently from cab4dc4 to c521fcb Compare July 24, 2024 13:47
src/fixed.c Outdated Show resolved Hide resolved
src/image-jpeg.c Outdated Show resolved Hide resolved
src/fixed.c Outdated Show resolved Hide resolved
src/fixed.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase the latest main branch to enable CI pipeline.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Improve the git commit messages by using complete sentences and making them informative.

src/fixed.c Outdated Show resolved Hide resolved
src/fixed.c Outdated Show resolved Hide resolved
@jserv
Copy link
Contributor

jserv commented Jul 25, 2024

You should avoid submitting pull requests via the main branch (or default branch). Instead, always create a dedicated git branch to work on specific features or bug fixes.

In this case, you should perform the following steps to resolve the unintended conflicts:

$ git remote add upstream https://github.com/sysprog21/mado
$ git fetch upstream
$ git reset --hard 65fa6e742a6bc16b31b6b41d3dec722857298a7f
$ git cherry-pick origin/main
$ git rebase upstream/main

After verifying via git log, do git push --force.

Improve your skills with Git by watching the videos: https://hackmd.io/@sysprog/git-with-github

src/fixed.c Outdated Show resolved Hide resolved
src/fixed.c Outdated Show resolved Hide resolved
@marvin0102 marvin0102 force-pushed the main branch 4 times, most recently from ac884a2 to 053eba6 Compare July 25, 2024 14:22
@jserv jserv requested a review from weihsinyeh July 25, 2024 14:24
if (as <= 0)
return 0;
if (CHECK_INTERVAL(as, TWIN_SFIXED_ONE, (1 << (2 - 1))))
return TWIN_SFIXED_ONE;
Copy link
Collaborator

@weihsinyeh weihsinyeh Jul 25, 2024

Choose a reason for hiding this comment

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

I'm curious about the reason for using CHECK_INTERVAL.
new_w
The following figure is the implementation without the CHECK_INTERVAL.
new_wo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is used to find an approximation of sin and cos values. However, this method makes it hard to calculate the square root when the numbers are near 1, which causes the wrong approximation of sin and cos values, especially when the radian is near pi/2 or 0.

Copy link
Collaborator Author

@marvin0102 marvin0102 Jul 26, 2024

Choose a reason for hiding this comment

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

The two figures below are sin and cos value calculted with (first) and without (second) CHECK_INTERVAL. Test on rv32emu/tests/line.c.
line
line_wrong

src/fixed.c Outdated Show resolved Hide resolved
src/fixed.c Outdated Show resolved Hide resolved
The current square root calculation for fixed-point numbers cannot
 handle values less than 1 accurately. To improve precision,
replace the existing method with the digit-by-digit calculation
method. This approach, combined with offset manipulation, will
minimize precision loss during the square root calculation.
@jserv jserv merged commit 2cfba0f into sysprog21:main Jul 26, 2024
3 checks passed
@jserv
Copy link
Contributor

jserv commented Jul 26, 2024

Thank @marvin0102 for contributing!

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