Skip to content

Math: Library: Add the hifi4 exponential function - #6801

Merged
kv2019i merged 1 commit into
thesofproject:mainfrom
ShriramShastry:Hifi4_Exponential_function_dev
Jun 16, 2023
Merged

Math: Library: Add the hifi4 exponential function#6801
kv2019i merged 1 commit into
thesofproject:mainfrom
ShriramShastry:Hifi4_Exponential_function_dev

Conversation

@ShriramShastry

@ShriramShastryShriramShastry commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

The 32-bit HiFi4 exponential library function has an accuracy of 1e-4, a unit in last place error of 5.60032793, and output ranges from 0.0067379470 to 148.4131591026 for inputs from -5 to +5 (Q4.28) (Q9.23).

Signed-off-by: ShriramShastry malladi.sastry@intel.com

image

@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch 7 times, most recently from 3ed16a6 to 131fc13CompareDecember 14, 2022 14:19

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment threadsrc/math/Kconfig Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Extra blanks after ae_int64 *. You could also avoid this function proto with move of mul_s64() here.

Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

declare variables in the begin of function

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure

Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Declare all variables in the beginning

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure

Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you compare qt directly to integer constant without need for qt_temp?

@ShriramShastryShriramShastryDec 26, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

qt temp = qt; is used to prevent the following error in the event that (qt < CONVERG_ERROR)
no operator overload defined for binary operator '<' (operand types 'ae_int64' (aka '_TIE_xt_hifi2_ae_int64') 'long long')
can you please suggest clean approach.

Comment threadtest/cmocka/src/math/arithmetic/exponential_hifi4.c Outdated
@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch 17 times, most recently from 51bc618 to 3465959CompareDecember 27, 2022 06:55
@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch from 3465959 to a771b4aCompareJanuary 3, 2023 15:31

@ShriramShastryShriramShastry left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thank you for looking at the code.
I incorporated the proposal and expressed my opinion.

Comment threadsrc/math/Kconfig Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadtest/cmocka/src/math/arithmetic/exponential_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/math/exp_fcn_hifi4.c Outdated

@ShriramShastryShriramShastryDec 26, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

qt temp = qt; is used to prevent the following error in the event that (qt < CONVERG_ERROR)
no operator overload defined for binary operator '<' (operand types 'ae_int64' (aka '_TIE_xt_hifi2_ae_int64') 'long long')
can you please suggest clean approach.

Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure

Comment threadsrc/math/exp_fcn_hifi4.c Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure

@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch from a771b4a to 8eab8d6CompareJanuary 4, 2023 06:08
@lgirdwood

Copy link
Copy Markdown
Member

@ShriramShastry btw, whats the speedup here vs generic C version ?

@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch 2 times, most recently from 9533b89 to cacde9dCompareJune 3, 2023 02:02
Comment threadsrc/math/exp_fcn_hifi4.c Outdated
Comment threadsrc/include/sof/math/exp_fcn_hifi4.h Outdated
Comment threadsrc/include/sof/math/exp_fcn_hifi4.h Outdated
Comment threadsrc/include/sof/math/exp_fcn_hifi4.h Outdated
Comment threadsrc/math/CMakeLists.txt Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This unrelated change could be in another commit with title indicating improvement of cmocka test coverage. I assume the old test with 256 points would work with hifi4 version too.

@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch from cacde9d to 954d65fCompareJune 9, 2023 09:13
Comment threadtest/cmocka/src/math/arithmetic/exponential.c Outdated
@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch 6 times, most recently from 04ebea6 to b4c6345CompareJune 9, 2023 16:21

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Comment threadsrc/math/CMakeLists.txt Outdated
@ShriramShastry

Copy link
Copy Markdown
ContributorAuthor

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Kconfig, is enabled or disabled by below code, In the moment, it is disabled, to enable make default n to y

config EXP_FIXED
bool "Exponential functions"
default n
help
By selecting this, the 32-bit sofm_exp_int32() function can be used to calculate
exponential values. With a maximum ulp of 5, an exponential function with
an input range of -5 to +5 gives positive numbers between 0.00673794699908547 and
148.413159102577. The precision of this function is 1e-4.

@ShriramShastry
ShriramShastryforce-pushed the Hifi4_Exponential_function_dev branch from b4c6345 to 751f887CompareJune 12, 2023 12:07
@ShriramShastry

Copy link
Copy Markdown
ContributorAuthor

Code looks ready to go, but the Kconfig addition/rename needs clarification. See comment inline.

Kconfig, is enabled or disabled by below code, In the moment, it is disabled, to enable make default n to y

config EXP_FIXED bool "Exponential functions" default n help By selecting this, the 32-bit sofm_exp_int32() function can be used to calculate exponential values. With a maximum ulp of 5, an exponential function with an input range of -5 to +5 gives positive numbers between 0.00673794699908547 and 148.413159102577. The precision of this function is 1e-4.

Done !! Renamed EXP_FIXED to MATH_EXP.
Thanks

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ShriramShastry , looks good now.

@ShriramShastry

Copy link
Copy Markdown
ContributorAuthor

Thanks @ShriramShastry , looks good now.

Thank you. Is there any other unresolved issue that needs to be addressed?

@kv2019i

Copy link
Copy Markdown
Collaborator

@cujomalainey Ok to proceed with this? You still have a requested-changes pending.

@cujomalainey

Copy link
Copy Markdown
Contributor

@cujomalainey Ok to proceed with this? You still have a requested-changes pending.

I'll re-review today

@cujomalainey

Copy link
Copy Markdown
Contributor

looks like my comment is still open, would be nice to get it fixed but i won't block on it

The 32-bit HiFi4 exponential library function has an accuracy of 1e-4,
a unit in last place error of 5.60032793, and output ranges from
0.0067379470 to 148.4131591026 for inputs from -5 to +5 (Q4.28) (Q9.23).
Signed-off-by: ShriramShastry <malladi.sastry@intel.com>
@ShriramShastry

Copy link
Copy Markdown
ContributorAuthor

looks like my comment is still open, would be nice to get it fixed but i won't block on it

Done !!

Sign up for freeto 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.

8 participants

@ShriramShastry@lgirdwood@singalsu@gkbldcig@kv2019i@cujomalainey@btian1@andrula-song