Math: library: Add an exponential function#6655
Conversation
ShriramShastry
force-pushed
the
Exponential_function_dev
branch
3 times, most recently
from
e61c90e to
d792fe0
Compare
paulstelian97
left a comment
paulstelian97
left a comment
There was a problem hiding this comment.
I am mildly concerned that we don't really have the motivation to include this. The code itself looks fine to me but why do we need it in the firmware?
ShriramShastry
force-pushed
the
Exponential_function_dev
branch
from
d792fe0 to
a0bd188
Compare
Hi @paulstelian97 We currently have the exponential function |
|
My concern is just what is going to use this new function. On my platform it's fine because we have the RAM but on some others having unused/dead code might be a concern due to limited resources. That's the only reason I'm asking. I'm not outright blocking it though -- if you can tell me of something that uses it (if it's proprietary code it's enough to just say that) then I will approve. |
In the moment, the suggested Although I hope to provide TIE ( HiFi3, HiFi4, and HiFi5 ) for current The proposed
|
ShriramShastry
force-pushed
the
Exponential_function_dev
branch
3 times, most recently
from
f7bbf53 to
67e4a81
Compare
ShriramShastry
requested review from
aborisovich,
dbaluta,
lbetlej,
lgirdwood,
marc-hb,
mmaka1,
plbossart and
singalsu
as code owners
ShriramShastry
requested review from
aiChaoSONG,
andrula-song,
btian1,
cujomalainey,
johnylin76,
juimonen and
mwasko
There was a problem hiding this comment.
During CI inspections, I notice mistakes
/home/runner/work/sof/sof/src/math/exp_fcn.c:75:43: error: suggest parentheses around comparison in operand of ‘!=’ [-Werror=parentheses]
75 | if (in_0 != 0LL && (in_1 != 0LL && (in_0 > 0LL != in_1 > 0LL))) {
| ~~~~~^~~~~
/home/runner/work/sof/sof/src/math/exp_fcn.c:75:59: error: suggest parentheses around comparison in operand of ‘!=’ [-Werror=parentheses]
75 | if (in_0 != 0LL && (in_1 != 0LL && ((in_0 > 0LL) != in_1 > 0LL))) {
| ~~~~~^~~~~
There was a problem hiding this comment.
if (in_0 != 0LL && in_1 != 0LL && (in_0 > 0LL) != (in_1 > 0LL))
has a lot fewer parentheses, does the same and shouldn't generate any warnings
There was a problem hiding this comment.
I'm afraid the advice given is still insufficient. The parenthesis continue to fail the CI strict check.
For reference I am adding output
CHECK: Unnecessary parentheses around 'in_0 > 0LL'
#[17](https://github.com/thesofproject/sof/actions/runs/3549190428/jobs/5961273777#step:5:18)1: FILE: src/math/exp_fcn.c:75:
+ if (in_0 != 0LL && in_1 != 0LL && (in_0 > 0LL) != (in_1 > 0LL)) {
CHECK: Unnecessary parentheses around 'in_1 > 0LL'
#171: FILE: src/math/exp_fcn.c:75:
+ if (in_0 != 0LL && in_1 != 0LL && (in_0 > 0LL) != (in_1 > 0LL)) {
There was a problem hiding this comment.
there you go. I was even too conservative. You can drop those last internal parentheses too then
There was a problem hiding this comment.
is it just me or are the two warnings above mutually contradictory?
It's hard to tell because this code has kept changing and the links to the logs have been lost but I think one warning is from checkpatch whereas the other warning is from the compiler in the Zephyr toolchain.
A compiler almost always wins over checkpatch for many reasons:
- A compiler understands the code much better
- A compiler is required, checkpatch is not
- "Checkpatch is not always right" https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
- Last but not least, a compiler warning would stick after this PR is merged!! A checkpatch warning will not.
There was a problem hiding this comment.
"Checkpatch is not always right" https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
checkpatch is more often wrong when using checkpatch --strict a.k.a. checkpatch --subjective.
In https://github.com/thesofproject/sof/actions/runs/3564456798/jobs/5988433936, only --subjective is complaining about parentheses. So we're good.
There was a problem hiding this comment.
checkpatch is more often wrong when using checkpatch --strict a.k.a. checkpatch --subjective.
I'm renaming --strict to --subjective in #6689 to make that more obvious. One-line change, please review.
There was a problem hiding this comment.
is it just me or are the two warnings above mutually contradictory? One is suggesting parentheses and another one recommends to remove them. Looks like we cannot have both
It doesn't work either ,
/home/runner/work/sof/sof/src/math/exp_fcn.c:92:44: error: suggest parentheses around comparison in operand of ‘!=’ [-Werror=parentheses]
92 | if (in_0 != 0 && in_1 != 0 && in_0 > 0 != in_1 > 0) {
| ~~~~~^~~
Do you have a more effective solution?
I would have to put up with warnings that were below code otherwise.
if (in_0 != 0 && in_1 != 0 && in_0 > 0 != (in_1 > 0)) {
There was a problem hiding this comment.
checkpatch is more often wrong when using checkpatch --strict a.k.a. checkpatch --subjective.
I'm renaming
--strictto--subjectivein #6689 to make that more obvious. One-line change, please review.
I'm a little confused about the difference between "strict" and "subjective," and while you have the fix in one [codestyle / checkpatch (--subjective)] place, unit tests and cmocka utests give errors that cause tests to fail.
Please include a detailed description of how the check criteria are handled in table style.
Thanks !!
ShriramShastry
force-pushed
the
Exponential_function_dev
branch
2 times, most recently
from
c2e0388 to
a05ca8d
Compare
cujomalainey
left a comment
cujomalainey
left a comment
There was a problem hiding this comment.
please run cppcheck on this
There was a problem hiding this comment.
this isn't needed since you overwrite it below before the next usage
There was a problem hiding this comment.
I note that accuracy is compromised while absin1 is failing to reset 0x4000000000000000.
index| input(Q4.28)| r_hw(Q9.23)| r_gold| THDN| ULP|
---------+---------------+---------------+---------------+---------------+---------------+
1|-4.921259842813| 11.02010691| 0.00728994| 20.83796843|110128.16973544|
There was a problem hiding this comment.
Are you saying this pointless absin1 = 0 line makes a difference? Can you share reproduction steps?
I compiled this file with and without this line commented out and the generated exp_fnc.c.obj files are identical. I tried both XCC and the Zephyr SDK 0.15.2
There was a problem hiding this comment.
I 'm saying this pointless absin1 = 0 line makes a difference. To reproduce , you can run the CI Unit test . I 've included step by step explanation here https://github.com/thesofproject/sof/pull/6655#discussion_r1052030947
There was a problem hiding this comment.
@ShriramShastry this could be a compiler error when trying to optimize the code since we all expect this assignment to be ignored (since its assigned again later on). You and @marc-hb could have different compiler versions and -O optimization flags. Also which platform are you testing on as the older platforms have older XCC versions and different ISAs.
There was a problem hiding this comment.
@ShriramShastry I cant see this in the new code, has this assignment issue now resolved ? Sometimes re-writing the function is enough to avoid the CC bug.
There was a problem hiding this comment.
I'm saying absin1 = 0 is removed, and I've provided an explanation if 'absin1 = 0' is removed.
Though I'm not sure where this 'absin1 = in0lo absin0? 1L: 0L' 'L' is seen by @cujomalainey and why we're talking about compilers (I can't comment because the present code is 'absin1 = in0lo absin0? 1: 0;'.
There was a problem hiding this comment.
@ShriramShastry its here line 122. You do a write without a read which just means its not needed. Compiler is likely throwing the instruction away anyways.
There was a problem hiding this comment.
OMG, I didn't realize this spurious absin1 = 0 assignment was still there, good catch @cujomalainey , thank you.
@ShriramShastry the reason we're talking about compilers is because every correct compiler with -O will discard this pointless line that does nothing. When I tried both the Xtensa XCC and the Zephyr SDK 0.15.2 toolchain above they both correctly discarded this pointless line of code. If you have found a compiler/configuration that does not discard this pointless line then you have found a very, very serious compiler issue that we MUST know about. So which exact toolchain does NOT discard this line?
In other words, please share the toolchain and command line that creates a different exp_fnc.c.obj file when you replace this line by // absin1 = 0. No explanation please, only the toolchain and command line that produce a different exp_fnc.c.obj
There was a problem hiding this comment.
done, Thank you
There was a problem hiding this comment.
lets simplify this a bit.
!= 0 is functionally the same as not so you can just use !
Your brackets are not doing anything right now, I am going to take a guess at what you meant to use them for
One possible optimization is doing a exclusive or on the signed bit, might save a few ops
`!in_0 && !in_1 && ((in_0 > 0) != (in_1 > 0))
There was a problem hiding this comment.
If you do plan to try to XOR the sign bits, you should add some casts to unsigned because C compilers can do weird shit if you XOR together signed numbers (in particular, comparisons might not work correctly due to optimizations)
There was a problem hiding this comment.
Compilers do crazy optimizations that break code relying on undefined behavior, that's a fact:
- https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow
- zephyrproject-rtos/zephyr@35bdab5c9a03
Signed representation and sign bits are not standard:
https://softwareengineering.stackexchange.com/questions/438560/in-c-how-is-signed-integer-stored
Does that mean compilers do crazy optimizations that break sign bits too? I don't know, hopefully not? Is it worth the risk? Probably not either.
There was a problem hiding this comment.
Thanks Marc, https://github.com/zephyrproject-rtos/zephyr/commit/35bdab5c9a03, can you please add the zephyr multiplication function link. Thank you
There was a problem hiding this comment.
Thanks Marc, https://github.com/zephyrproject-rtos/zephyr/commit/35bdab5c9a03, can you please add the zephyr multiplication function link.
This commit was about signed overflows, unrelated to multiplication.
There was a problem hiding this comment.
@ShriramShastry this isn't resolved, im with marc, this should be done above
ShriramShastry
left a comment
ShriramShastry
left a comment
There was a problem hiding this comment.
Thank you for looking at the code.
Please see my replies.
There was a problem hiding this comment.
Sure, Included a random number test with ~-/+5 range.
There was a problem hiding this comment.
Unfortunately ((qt & QUOTIENT_SCALE) >> 30)) doesn't converge for a 4.68503937.
I see accuracy check FAIL
There was a problem hiding this comment.
Sure. I will write Q notation article in SOF docs.
There was a problem hiding this comment.
bool operator to unsigned int32 results in accuracy issue. I see during exit iteration 18 following data
PASS FAIL
|Variable |value |type |value |type |
+---------------+-----------------------+----------------+----------------------+-----------------------+
|roundup |false |bool |0 |unsigned int |
|a |1137486160143636968 |__int64 | -80630 | __int64 |
|b |-1321040630 |__int64 | -1321040630 |__int64 |
|u64_rlo |12848879112039096761 |unsigned __int64| 396801 | unsigned __int64|
|u64_rhi |18446744073628091959 |unsigned __int64| 0 |unsigned __int64 |
|qt |8678330689572 |__int64 | 8686788985540 |__int64 |
|ts |61179 |int | 74156 |int |
|x |-1321040630 |int | -1321040630 |int |
|dividend |-5597864961670454855 |__int64 | -5603320895573891245 |__int64 |
|ou0Hi |0 |unsigned __int64| 0 |unsigned __int64 |
|ou0Lo |4549944640574547872 |unsigned __int64| 4554379223650996648 |unsigned __int64 |
|accum |61179 |int |74156 |int |
index| input(Q4.28)| r_hw(Q9.23)| r_gold| THDN| ULP|
-----+---------------+---------------+---------------+---------------+---------------+
FAIL|-4.921259842813| 0.00884008| 0.00728994| -56.19256363| 15.50143197|
PASS|-4.921259842813| 0.00729311| 0.00728994| -109.99459519| 0.03164246|
There was a problem hiding this comment.
I note that accuracy is compromised while absin1 is failing to reset 0x4000000000000000.
index| input(Q4.28)| r_hw(Q9.23)| r_gold| THDN| ULP|
---------+---------------+---------------+---------------+---------------+---------------+
1|-4.921259842813| 11.02010691| 0.00728994| 20.83796843|110128.16973544|
There was a problem hiding this comment.
Comments must say something the reader does not already know.
There was a problem hiding this comment.
Are you saying this pointless absin1 = 0 line makes a difference? Can you share reproduction steps?
I compiled this file with and without this line commented out and the generated exp_fnc.c.obj files are identical. I tried both XCC and the Zephyr SDK 0.15.2
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Getting close, a lot of review comments updated.
There was a problem hiding this comment.
For this SOF_MATH_EXP_ would be good. Also the macros those are used only by the single C module could be in the module to avoid defining non-useful macros for applications space using exp().
There was a problem hiding this comment.
SOF_MATH_EXPONENTIAL_H is OK ?
There was a problem hiding this comment.
This should be in the C file ?
There was a problem hiding this comment.
Seppo suggests that C and HiFi versions share a similar header file.
I believe it is beneficial to consider
There was a problem hiding this comment.
That's fine to share the same header, but any user of this symbol(table) would duplicate it in each object file. Intention of the math library is to provide a set of utility functions for use by processing modules (i.e. we just want a single copy of the table in the code) so this is better in the C file that exposes the public API.
There was a problem hiding this comment.
I'm a little bewildered and would be glad with your advice.
My question is - You want an extra file with a lookup table ? Or restore the lookup table in the current exp fcn.c file ?
There was a problem hiding this comment.
@singalsu suggests that C and HiFi versions share a similar header file.
I don't understand this sorry, could you please elaborate?
-
How does "Similar .h file" imply "Must have
iv[]in .h file" ? -
What is the "HiFi version"? AFAIK "HiFi" is a product name and C is a language; I don't understand how they can share a .h file.
There was a problem hiding this comment.
The lookup table and constant are the identical for the HiFi #6801 and exponential C versions.
There is a conversation on preventing code duplication.
There was a problem hiding this comment.
The lookup table and constant are the identical for the HiFi #6801 and exponential C versions.
Thanks!
There is a conversation on preventing code duplication.
Move to .c file and drop static, done?
There was a problem hiding this comment.
@ShriramShastry this could be a compiler error when trying to optimize the code since we all expect this assignment to be ignored (since its assigned again later on). You and @marc-hb could have different compiler versions and -O optimization flags. Also which platform are you testing on as the older platforms have older XCC versions and different ISAs.
There was a problem hiding this comment.
some of these additional brackets are unneeded and just make it harder to read
There was a problem hiding this comment.
I see errors
error: suggest parentheses around comparison in operand of ‘!=’ [-Werror=parentheses]
98 | if (in_0 && (in_1 && (in_0 > 0 != in_1 > 0))) {
| ~~~~~^~~
There was a problem hiding this comment.
error: suggest parentheses around comparison in operand of ‘!=’ [-Werror=parentheses]
98 | if (in_0 && (in_1 && (in_0 > 0 != in_1 > 0))) {
You tried to remove the only parentheses that seem useful. I don't want to be guessing the precedence between > and !=. It should be possible to remove the other parentheses without issue, hopefully like this:
| if (in_0 && (in_1 && ((in_0 > 0) != (in_1 > 0)))) { | |
| if (in_0 && in_1 && (in_0 > 0) != (in_1 > 0)) { |
There was a problem hiding this comment.
Nit:
| bool roundup = (u64_rlo & BIT_MASK_LOW_Q27P5) != 0; | |
| const bool roundup = (u64_rlo & BIT_MASK_LOW_Q27P5) != 0; |
const all constants as a best practice, it's more readable and catch future bugs. const is the default in some modern languages.
There was a problem hiding this comment.
@singalsu suggests that C and HiFi versions share a similar header file.
I don't understand this sorry, could you please elaborate?
-
How does "Similar .h file" imply "Must have
iv[]in .h file" ? -
What is the "HiFi version"? AFAIK "HiFi" is a product name and C is a language; I don't understand how they can share a .h file.
ShriramShastry
left a comment
ShriramShastry
left a comment
There was a problem hiding this comment.
I believe all of the concerns have been addressed.
Please specify any outstanding ones.
Thank you very much.
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Minor changes requested
There was a problem hiding this comment.
Add a comment explaining why this cast is required or remove it.
There was a problem hiding this comment.
Where is / what is a?
| /* f(x) = a^x, x is variable and a is base | |
| /* f(x) = e^x |
There was a problem hiding this comment.
functions of the form f(x) = a ^ x , it goes as f(x) = exp(x) so, it is not necessary to alter the current depiction.
There was a problem hiding this comment.
The "magic" values seem very specific to exp_int32() and used only once there but they're very far away from where they are used and there is nothing in their name that links them to exp_int32().
If they are indeed specific to exp_int32() then simply declare them as const inside exp_int32(); no need to pollute the entire file's namespace with very generic names.
There was a problem hiding this comment.
It's not magical, but if a user follows the fixed point formatting style, they can readily understand how the number was calculated. For ease of usage, I've added a remark describing how an input value that is outside of the acceptable range results in clipped output.
There was a problem hiding this comment.
Thanks for adding a comment but my request was not at all about the values, please read it again. It was 100% about WHERE they values are located in the file and HOW they are defined.
|
@ShriramShastry I think we are close now, but pls do run cppcheck on this file. If you dont have cppcheck, you can install via |
I 've executed the |
ShriramShastry
left a comment
ShriramShastry
left a comment
There was a problem hiding this comment.
Include the recommendation.
Thanks
There was a problem hiding this comment.
It's not magical, but if a user follows the fixed point formatting style, they can readily understand how the number was calculated. For ease of usage, I've added a remark describing how an input value that is outside of the acceptable range results in clipped output.
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
The now global iv is a problem, see below.
I 've executed the cppcheck and I think they can be ignored.
The following warnings seem very valid and very easy to fix:
[/sof/test/cmocka/src/math/arithmetic/exponential.c:49] (style) Condition 'b2<4503599627370496' is always true [knownConditionTrueFalse]
[/sof/test/cmocka/src/math/arithmetic/exponential.c:46] (style) Variable 'a2' is reassigned a value before the old one has been used. [redundantAssignment]
[/sof/test/cmocka/src/math/arithmetic/exponential.c:47] (style) Variable 'b2' is reassigned a value before the old one has been used. [redundantAssignment]
This one does not really matter:
[/sof/test/cmocka/src/math/arithmetic/exponential.c:61] (style) The scope of the variable 'accum' can be reduced.
There was a problem hiding this comment.
Thanks for adding a comment but my request was not at all about the values, please read it again. It was 100% about WHERE they values are located in the file and HOW they are defined.
There was a problem hiding this comment.
Now that this array is being re-used across multiple files, this name iv is visible across the entire binary. So it must be longer now to avoid collisions and to make it possible to find in the source code. Right now git grep iv returns more than 4000 lines.
Maybe something like math_exp_inv[]? Anything with some kind of namespace prefix.
Sorry I'm realizing this only now, I should have said earlier.
There was a problem hiding this comment.
As reported when you ran ccpcheck, what is the purpose of this line? It has no effect.
There was a problem hiding this comment.
done!!
The test was rerun, and the cppcheck results are presented below.
I don't notice any prior warnings.
I'm curious if every PR passes the cppcheck, and if not, what criteria is used ?
[/sof/test/cmocka/src/math/arithmetic/exponential.c:61] (style) The scope of the variable 'accum' can be reduced. Warning: Be careful when fixing this message, especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:
void f(int x)
{
int i = 0;
if (x) {
// it's safe to move 'int i = 0;' here
for (int n = 0; n < 10; ++n) {
// it is possible but not safe to move 'int i = 0;' here
do_something(&i);
}
}
}
When you see this message it is always safe to reduce the variable scope 1 level. [variableScope]
[/sof/test/cmocka/src/math/arithmetic/exponential.c:63] (style) The scope of the variable 'a_i' can be reduced. Warning: Be careful when fixing this message, especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:
void f(int x)
{
int i = 0;
if (x) {
// it's safe to move 'int i = 0;' here
for (int n = 0; n < 10; ++n) {
// it is possible but not safe to move 'int i = 0;' here
do_something(&i);
}
}
}
When you see this message it is always safe to reduce the variable scope 1 level. [variableScope]
[/sof/test/cmocka/src/math/arithmetic/exponential.c:64] (style) The scope of the variable 'max_ulp' can be reduced. Warning: Be careful when fixing this message, especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:
void f(int x)
{
int i = 0;
if (x) {
// it's safe to move 'int i = 0;' here
for (int n = 0; n < 10; ++n) {
// it is possible but not safe to move 'int i = 0;' here
do_something(&i);
}
}
}
When you see this message it is always safe to reduce the variable scope 1 level. [variableScope]
ShriramShastry
left a comment
ShriramShastry
left a comment
There was a problem hiding this comment.
Done , Please have a look. Thank you
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
This is now failing to compile with xt-xcc, see below. Quickbuild was broken for a while so maybe that's why we did not notice yet.
There was a problem hiding this comment.
iv_t is easier to grep than iv so this is progress but it's still a very short and super cryptic name for a global constant.
There was a problem hiding this comment.
Quickbuild is back - with a vengeance. This fails to compile with xt-xcc in quickbuild, see error below.
Welcome to integer promotion rules in C!
The error is unfortunately visible only on the internal Intel server since we had to migrate the public 01.org to new hosting. The internal build number is build/11377029
09:17:28,736 INFO - cd /quickbuild/workspace1/24733/SOF_FW/build_ut/test/cmocka/src/math/arithmetic && /localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc -DBLD_COUNTERS=0 -DCONFIG_NUMBERS_NORM -DCONFIG_NUMBERS_VECTOR_FIND -DUNIT_TEST -D_UINTPTR_T_DEFINED -DRELATIVE_FILE=\"test/cmocka/src/math/arithmetic/base_e_logarithm.c\" -I/quickbuild/workspace1/24733/SOF_FW/test/cmocka/include -I/localdisk/tools/cmocka/build_cavs2x_LX6HiFi3_2017_8/install/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/xtos -I/quickbuild/workspace1/24733/SOF_FW/xtos/include -I/quickbuild/workspace1/24733/SOF_FW/src/platform/tigerlake/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/arch/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/include -I/quickbuild/workspace1/24733/SOF_FW/src/platform/intel/cavs/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtos-wrapper/include -I/quickbuild/workspac...
09:17:28,815 INFO - cc1: warnings being treated as errors
09:17:28,815 INFO - cmocka/src/math/arithmetic/exponential.c: In function ‘gen_exp_testvector’:
09:17:28,815 INFO - cmocka/src/math/arithmetic/exponential.c:45: warning: integer constant is too large for ‘long’ type
09:17:28,815 INFO - cmocka/src/math/arithmetic/exponential.c:48: warning: this decimal constant is unsigned only in ISO C90
09:17:28,815 INFO - cmocka/src/math/arithmetic/exponential.c:49: warning: this decimal constant is unsigned only in ISO C90
09:17:28,818 INFO - cmocka/src/math/arithmetic/CMakeFiles/exponential.dir/build.make:76: recipe for target 'test/cmocka/src/math/arithmetic/CMakeFiles/exponential.dir/exponential.c.o' failed
There was a problem hiding this comment.
should be corrected!
Can you confirm, please?
Thanks
Running cppcheck on every PR would be great. It would require listing which files are modified by the PR, otherwise running on all files would produce a very long list of warnings that, too long for anyone to look at. I implemented a script that does in https://github.com/thesofproject/sof-test/blob/main/tools/CI/check-gitrange.bash but it would need to be converted to a proper github action and I have no idea how to do this. I searched for existing github actions and found none that looked decent. |
Is there any other pending items or feedback to share? Thank you. |
|
quickbuild build/11382559 is still failing to compile with similar errors. Did you compile with xt-xcc? |
|
@ShriramShastry looks like CI is still failing, pls check with @singalsu if blocked. |
Done, Thanks Seppo !! |
andyross
left a comment
andyross
left a comment
There was a problem hiding this comment.
Marc reminded me about this PR. It seems like most of my clarity concerns were addressed, and what remains amounts mostly to nitpicking about code size or performance. I don't see any reason to hold this up.
There was a problem hiding this comment.
please change this to sofm_exp_int32()
There was a problem hiding this comment.
Done !! Thank you
|
SOFCI TEST |
There was a problem hiding this comment.
I assume this needs to be static ?
There was a problem hiding this comment.
@lgirdwood no, this will be shared with the next pull request.
There was a problem hiding this comment.
OMG, I didn't realize this spurious absin1 = 0 assignment was still there, good catch @cujomalainey , thank you.
@ShriramShastry the reason we're talking about compilers is because every correct compiler with -O will discard this pointless line that does nothing. When I tried both the Xtensa XCC and the Zephyr SDK 0.15.2 toolchain above they both correctly discarded this pointless line of code. If you have found a compiler/configuration that does not discard this pointless line then you have found a very, very serious compiler issue that we MUST know about. So which exact toolchain does NOT discard this line?
In other words, please share the toolchain and command line that creates a different exp_fnc.c.obj file when you replace this line by // absin1 = 0. No explanation please, only the toolchain and command line that produce a different exp_fnc.c.obj
The 32-bit exponential library function has an accuracy of 1e-4 and a unit in last place error of 4.5878 for inputs from -5 to +5 (Q4.28) and outputs from 0.0067379470 to 148.4131591026 (Q9.23). Signed-off-by: ShriramShastry <malladi.sastry@intel.com>
|
Is there anything open that still needs to be evaluated that I need to fix ? |
|
I don't think any of the 3 check fails are related to this PR. The one code style check is about (), but I recall the discussion outcome was that those are recommended to keep. It would be good to get this exponent function PR merged first to cleanly proceed with HiFi optimized versions. |
11 participants
The 32-bit exponential library function has an accuracy of 1e-4 and a unit in last place error of 5.60032793 for inputs from -5 to +5 (Q4.28) and outputs from 0.0067379470 to 148.4131591026 (Q9.23).
Signed-off-by: ShriramShastry malladi.sastry@intel.com