Make float test more accurate, add integer performance tests. #217
Conversation
Add integer square root performance tests
tests/system/hw_math/source/main.c
Outdated
| @@ -7,6 +7,82 @@ | |||
|
|
|||
| #include <nds.h> | |||
|
|
|||
| #define TEST_SIZE (1000) | |||
|
|
|||
| u32 dkp_sqrt32(int a) | |||
There was a problem hiding this comment.
Please avoid putting copy-pasted benchmarks for other projects in BlocksDS. Not only can they change the code at any time, which would make the copy paste outdated, it feels in bad taste.
This can be a separate project.
There was a problem hiding this comment.
I'll remove them then. I could put in a software implementation test instead and rename the output.
There was a problem hiding this comment.
Yeah, measuring against a simple C-based implementation sounds like a better idea and provides meaningful information to developers; it can also be used to validate correct outputs from the BlocksDS hardware accelerated implementation.
tests/system/hw_math/source/main.c
Outdated
| @@ -7,6 +7,82 @@ | |||
|
|
|||
| #include <nds.h> | |||
|
|
|||
| #define TEST_SIZE (1000) | |||
|
|
|||
| u32 dkp_sqrt32(int a) | |||
(note that these are signed, I havent found an unsigned version yet) also speed depends on input size they can also be optimized with __builtin_clz in some cases, but wont do that today
There was a problem hiding this comment.
You really need to try to match the code style of the repository when you contribute to projects.
In the case of this repository, don't leave many empty lines between functions (one is enough), try to match where braces are opened and closed, capitalise comments correctly... Take a look at other PRs and you will see that, for the most part, contributors imitate pre-existing code. I'm not going to complain about one brace here and there not matching other code, but this is too different, and I would just have to clean it up myself.
|
|
||
| sw_time += cpuEndTiming(); | ||
|
|
||
| printf("BlocksDS sqrt64 time: %lu (%lu per op)\n", sw_time, sw_time / sw_iterations); |
There was a problem hiding this comment.
This won't fit in one line, you should split it into two so that numbers aren't cut in the middle.
| //which gives the source as | ||
| //Square root by abacus algorithm, Martin Guy @ UKC, June 1985. | ||
| //From a book on programming abaci by Mr C. Woo. | ||
| //assert(("sqrt input should be non-negative", n > 0)); |
There was a problem hiding this comment.
Don't leave this commented assert(). You should just make the function take unsigned ints as input and return unsigned ints.
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Don't add too many spaces, indent blocks correctly.
| @@ -49,6 +235,8 @@ int main(int argc, char **argv) | |||
| printf("qNaN: 0x%08lX\n", quiet_nan); | |||
| printf("\n"); | |||
There was a problem hiding this comment.
Don't add (or remove) empty lines for no reason, they just add noise to the git history.
|
I plan to work on this further, I wouldn't consider it finished. I assume to reduce noise/you getting unnecessary notifications it's probably not enough to mark it as a draft and request a review manually when necessary. The option I have here is either to close the PR until it's done, or to split the changes, although then I'm not sure why the draft feature exists to begin with. |
|
The draft feature is fine; just keep us updated on the status. If you ever feel like the PR is not up to standard, feel free to close it. |
This doesn't contain an accuracy test for the integer square root functions, but I could add one if needed.