Ayopierre/libm#507
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * | ||
| * %LICENSE% | ||
| */ | ||
|
|
There was a problem hiding this comment.
[clang-format-pr] reported by reviewdog 🐶
suggested fix
| For documentation please refer to /docs directory present on the main repo linked above. | ||
|
|
||
| # Phoenix | ||
| This is implementation of math library developed alongside Pheonix RTOS operating system. Keep in mind that this implementation is not complete, not all functions in C99 standard are implemented. |
There was a problem hiding this comment.
[codespell] reported by reviewdog 🐶
Pheonix ==> Phoenix
|
|
||
| # Notes | ||
| 1. This is a temporary state, moving math library to be separate to libphoenix | ||
| 2. Some hardware instrinsics for math functions are still not implemented |
There was a problem hiding this comment.
[codespell] reported by reviewdog 🐶
instrinsics ==> intrinsics
| init->i.mantisa |= (0x1ull << 51); | ||
| } | ||
|
|
||
| /* Reciprocal sqare root iters (avoiding division): */ |
There was a problem hiding this comment.
[codespell] reported by reviewdog 🐶
sqare ==> square
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the math library by moving it from libphoenix to a standalone libm directory, introducing architecture-specific hardware-accelerated implementations alongside a software-based fallback. The review identified several critical issues, including a logic error in sinhf, a function name typo in truncf, a C99 linkage issue with inline functions, a syntax error in nanf, and potential build failures in the Makefile. Additionally, minor typos in the documentation were addressed.
| float sinhf(float x) | ||
| { | ||
| return (double)sin((double)x); | ||
| } |
There was a problem hiding this comment.
The implementation of sinhf incorrectly calls sin (trigonometric sine) instead of sinh (hyperbolic sine). This will return completely incorrect results for hyperbolic sine calculations.
float sinhf(float x)
{
return (float)sinh((double)x);
}|
|
||
| #include <math.h> | ||
|
|
||
|
|
There was a problem hiding this comment.
The function name is misspelled as trunf instead of truncf. This will cause linker errors or prevent the hardware-accelerated version of truncf from being used.
float truncf(float x)| #include <errno.h> | ||
|
|
||
|
|
||
| inline double sqrt(double x) |
There was a problem hiding this comment.
Defining sqrt as inline in a .c file without static or extern means that under C99 rules, no external symbol is emitted for sqrt. This will cause 'undefined reference to sqrt' errors at link time. The inline keyword should be removed.
double sqrt(double x)| float nanf(const char *) | ||
| { | ||
| return __builtin_nanf(""); | ||
| } |
There was a problem hiding this comment.
In C, omitting the parameter name in a function definition is a syntax error. An identifier must be provided for the parameter, even if it is unused.
| float nanf(const char *) | |
| { | |
| return __builtin_nanf(""); | |
| } | |
| float nanf(const char *tagp) | |
| { | |
| return __builtin_nanf(""); | |
| } |
| cp -a include/* "$(HEADERS_INSTALL_DIR)"; \ | ||
| cp -a libm/libmcs/libm/include/* "$(HEADERS_INSTALL_DIR)"; |
There was a problem hiding this comment.
If the libm/libmcs submodule is not checked out or initialized, the directory libm/libmcs/libm/include will not exist, causing the cp -a command to fail and halt the installation. It is safer to conditionally copy the headers only if the directory exists.
cp -a include/* "$(HEADERS_INSTALL_DIR)"; \
if [ -d libm/libmcs/libm/include ]; then cp -a libm/libmcs/libm/include/. "$(HEADERS_INSTALL_DIR)"; fi
| @@ -0,0 +1,38 @@ | |||
| # Libm | |||
| This directory contains implementation of math library. It currently links to libphoenix with intention to be compiled to separate libm.a library. There are two implementation for two follo> | |||
There was a problem hiding this comment.
The sentence is cut off at the end with follo>. It should be completed (e.g., following implementations:).
| This directory contains implementation of math library. It currently links to libphoenix with intention to be compiled to separate libm.a library. There are two implementation for two follo> | |
| This directory contains implementation of math library. It currently links to libphoenix with intention to be compiled to separate libm.a library. There are two implementations for the following cases: |
| For documentation please refer to /docs directory present on the main repo linked above. | ||
|
|
||
| # Phoenix | ||
| This is implementation of math library developed alongside Pheonix RTOS operating system. Keep in mind that this implementation is not complete, not all functions in C99 standard are implemented. |
There was a problem hiding this comment.
Typo: Pheonix should be Phoenix.
| This is implementation of math library developed alongside Pheonix RTOS operating system. Keep in mind that this implementation is not complete, not all functions in C99 standard are implemented. | |
| This is implementation of math library developed alongside Phoenix RTOS operating system. Keep in mind that this implementation is not complete, not all functions in C99 standard are implemented. |
Unit Test Results11 307 tests ±0 10 583 ✅ - 17 1h 1m 27s ⏱️ + 8m 6s For more details on these failures, see this check. Results for commit c948336. ± Comparison against base commit fb3245b. ♻️ This comment has been updated with latest results. |
…ate files YT: RTOS-1132
YT: RTOS-1132
YT: RTOS-1132
aeed5b9 to
c948336
Compare
1 participant
Add secondary math libary implementation
Description
Add new math library implementation, with selection which version to build and selection for usage of hw intrinsics for math operations.
Motivation and Context
Libmcs provides complete C99 math library implementation.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment