Math: replaces the gcd algorithm #2381

IulianOlaru249 · 2020-02-13T11:24:59Z

This patch replaces the former gcd algorithm which was implemented with
repetitive divisions with Stein's algorithm, whitch is a faster gcd
algorithm. All operations are arithmetic shifts, comparisons and
subtractions.

Signed-off-by: Iulian Olaru [email protected]

dbaluta · 2020-02-13T11:39:13Z

I wouldn't go that far without a real need. Looks like gcd is only used in 2 files and we don't have anyone complaining that gcd is slow.

Current algorithm is easier to understand, is proved to work. So, NACK from me :). Please go for low hanging fruits now.

paulstelian97

Not approving either, this introduces unneeded complexity. GCD is never called in any hot path so the additional performance (if it even is there) isn't worth the additional complexity. Code must be understandable by humans when there isn't a real benefit to the extra complexity.

jajanusz · 2020-02-13T12:51:41Z

If @singalsu is going to use GCD for some fancy processing we may keep it. Also @IulianOlaru249 would need to remove note about wikipedia from line # 9

singalsu · 2020-02-13T14:45:28Z

The function gcd() is not used in execution time critical loops, only in initialization code. I'm not aware of such use for it.

@IulianOlaru249 Does some of your development depend on faster version?

IulianOlaru249 · 2020-02-13T14:55:44Z

@singalsu , not really. I was looking trough the code and thought of the algorithm.

dbaluta · 2020-02-13T15:01:31Z

Closing this, thanks @IulianOlaru249 it was a really nice idea. We will revive it if at some point will see in tests that gcd is slowing us down.

lgirdwood

This is a win for me since it removes the % which is expensive on xtensa and other RISC architectures. We should also not assume gcd will only be used in cold paths. I would add extra comments with maybe a link to the algorithm.

paulstelian97 · 2020-02-14T11:25:15Z

This is a win for me since it removes the % which is expensive on xtensa and other RISC architectures. We should also not assume gcd will only be used in cold paths. I would add extra comments with maybe a link to the algorithm.

Oh. Alright I see your point, then I guess I'll suggest reopening and once those extra comments for clarification are in there I will approve as well.

jajanusz · 2020-02-14T11:58:07Z

@IulianOlaru249 Please also change replaces to replace in commit title. We have imperative mood in our titles. Thanks

lgirdwood · 2020-02-20T11:36:44Z

@singalsu can you review.

src/math/numbers.c

dbaluta · 2020-02-20T11:53:02Z

src/math/numbers.c

+ * If both parameters are 0, gcd(0, 0) returns 0
+ * If first parameters is 0 or second parameter is 0, gcd(0, b) returns b
+ * and gcd(a, 0) returns a, because everything devides 0.
+ * Link to algorithm:


I would add the link to algorithm at the beginning of the file. Like it was until now.

src/math/numbers.c

singalsu · 2020-02-20T12:52:53Z

@IulianOlaru249 How have you tested this? The test should include negative and positive numbers and specials like 0 and 1.

dbaluta · 2020-02-20T13:53:14Z

@singalsu @IulianOlaru249 I think we can enhance the mocking tests right?

test/cmocka/src/math/numbers/gcd.c

singalsu · 2020-02-20T14:05:39Z

@dbaluta That's a good idea. Can you @IulianOlaru249 please add a commit for more test cases.

test/cmocka/src/math/numbers/gcd.c

lgirdwood

Sorry @IulianOlaru249 ignore my previous approval, the last change is good, but these review fixes should be squashed into the top level feature patches.

lgirdwood

I think we are good to merge once comments and commit message are updated. Code is fine.

This patch replaces the former gcd algorithm which was implemented with repetitive divisions with Stein's algorithm, whitch is a faster gcd algorithm. All operations are arithmetic shifts, comparisons and subtractions. Signed-off-by: Iulian Olaru <[email protected]>

This patch adds extra tests for the gcd of (0, 0), (a, 0), (0, b), (-a, -b), (-a, b), (a, -b) Signed-off-by: Iulian Olaru <[email protected]>

dbaluta · 2020-03-19T18:07:17Z

src/math/numbers.c

@@ -5,6 +5,9 @@
 // Author: Seppo Ingalsuo <[email protected]>
 //         Liam Girdwood <[email protected]>
 //         Keyon Jie <[email protected]>
+/* Binary GCD algorithm
+ * https://en.wikipedia.org/wiki/Binary_GCD_algorithm
+ */



It looks these lines above are added twice. They shouldn't be present in this patch.

src/math/numbers.c

This patch makes the binary gcd algorithm work for negative numbers by turning them into positive ones. Works since gcd(a, b) = gcd(-a, -b) = gcd(-a, b) = gcd(a, -b) Signed-off-by: Iulian Olaru <[email protected]>

lgirdwood · 2020-03-20T11:33:15Z

Jenkins CI known issues. @wwittbrx @lbetlej any clues on internal CI, I cant see the problem.

jajanusz · 2020-03-25T15:40:25Z

@lgirdwood re-review pls

lgirdwood · 2020-03-26T12:12:13Z

CI jenkins known issues.

IulianOlaru249 requested a review from singalsu as a code owner February 13, 2020 11:24

IulianOlaru249 requested a review from dbaluta February 13, 2020 11:26

paulstelian97 requested changes Feb 13, 2020

View reviewed changes

dbaluta closed this Feb 13, 2020

lgirdwood reviewed Feb 14, 2020

View reviewed changes

paulstelian97 reopened this Feb 14, 2020

IulianOlaru249 force-pushed the math-01-patch branch from 475fcd4 to 6869c81 Compare February 20, 2020 11:21

dbaluta reviewed Feb 20, 2020

View reviewed changes

src/math/numbers.c Show resolved Hide resolved

dbaluta reviewed Feb 20, 2020

View reviewed changes

src/math/numbers.c Show resolved Hide resolved

dbaluta reviewed Feb 20, 2020

View reviewed changes

IulianOlaru249 force-pushed the math-01-patch branch from 6869c81 to e0d5159 Compare February 20, 2020 12:37

dbaluta reviewed Feb 20, 2020

View reviewed changes

src/math/numbers.c Outdated Show resolved Hide resolved

dbaluta reviewed Feb 20, 2020

View reviewed changes

src/math/numbers.c Show resolved Hide resolved

IulianOlaru249 requested a review from jajanusz as a code owner February 22, 2020 09:35

paulstelian97 reviewed Feb 22, 2020

View reviewed changes

test/cmocka/src/math/numbers/gcd.c Outdated Show resolved Hide resolved

lgirdwood approved these changes Feb 26, 2020

View reviewed changes

lgirdwood requested changes Feb 26, 2020

View reviewed changes

IulianOlaru249 force-pushed the math-01-patch branch from 3ce1f90 to 7f5ebfe Compare February 28, 2020 09:16

lgirdwood reviewed Feb 28, 2020

View reviewed changes

IulianOlaru249 added 2 commits March 19, 2020 18:59

test: Add extra tests fot gcd

bfa0c97

This patch adds extra tests for the gcd of (0, 0), (a, 0), (0, b), (-a, -b), (-a, b), (a, -b) Signed-off-by: Iulian Olaru <[email protected]>

IulianOlaru249 force-pushed the math-01-patch branch from 7f5ebfe to 2b42951 Compare March 19, 2020 17:01

dbaluta reviewed Mar 19, 2020

View reviewed changes

IulianOlaru249 force-pushed the math-01-patch branch from 2b42951 to feaaa2e Compare March 19, 2020 18:59

paulstelian97 reviewed Mar 19, 2020

View reviewed changes

src/math/numbers.c Outdated Show resolved Hide resolved

paulstelian97 reviewed Mar 19, 2020

View reviewed changes

src/math/numbers.c Outdated Show resolved Hide resolved

paulstelian97 reviewed Mar 19, 2020

View reviewed changes

src/math/numbers.c Outdated Show resolved Hide resolved

paulstelian97 requested changes Mar 19, 2020

View reviewed changes

src/math/numbers.c Outdated Show resolved Hide resolved

math: Make binary gcd work with negatives

be5516a

This patch makes the binary gcd algorithm work for negative numbers by turning them into positive ones. Works since gcd(a, b) = gcd(-a, -b) = gcd(-a, b) = gcd(a, -b) Signed-off-by: Iulian Olaru <[email protected]>

IulianOlaru249 force-pushed the math-01-patch branch from feaaa2e to be5516a Compare March 19, 2020 21:42

paulstelian97 approved these changes Mar 20, 2020

View reviewed changes

jajanusz requested a review from lgirdwood March 25, 2020 15:40

lgirdwood approved these changes Mar 26, 2020

View reviewed changes

lgirdwood merged commit 0bd34f5 into thesofproject:master Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Math: replaces the gcd algorithm #2381

Math: replaces the gcd algorithm #2381

IulianOlaru249 commented Feb 13, 2020

dbaluta commented Feb 13, 2020 •

edited

Loading

paulstelian97 left a comment

jajanusz commented Feb 13, 2020 •

edited

Loading

singalsu commented Feb 13, 2020

IulianOlaru249 commented Feb 13, 2020

dbaluta commented Feb 13, 2020

lgirdwood left a comment

paulstelian97 commented Feb 14, 2020

jajanusz commented Feb 14, 2020

lgirdwood commented Feb 20, 2020

dbaluta Feb 20, 2020

singalsu commented Feb 20, 2020

dbaluta commented Feb 20, 2020

singalsu commented Feb 20, 2020

lgirdwood left a comment

lgirdwood left a comment

dbaluta Mar 19, 2020

lgirdwood commented Mar 20, 2020

jajanusz commented Mar 25, 2020

lgirdwood commented Mar 26, 2020

Math: replaces the gcd algorithm #2381

Math: replaces the gcd algorithm #2381

Conversation

IulianOlaru249 commented Feb 13, 2020

dbaluta commented Feb 13, 2020 • edited Loading

paulstelian97 left a comment

Choose a reason for hiding this comment

jajanusz commented Feb 13, 2020 • edited Loading

singalsu commented Feb 13, 2020

IulianOlaru249 commented Feb 13, 2020

dbaluta commented Feb 13, 2020

lgirdwood left a comment

Choose a reason for hiding this comment

paulstelian97 commented Feb 14, 2020

jajanusz commented Feb 14, 2020

lgirdwood commented Feb 20, 2020

dbaluta Feb 20, 2020

Choose a reason for hiding this comment

singalsu commented Feb 20, 2020

dbaluta commented Feb 20, 2020

singalsu commented Feb 20, 2020

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

dbaluta Mar 19, 2020

Choose a reason for hiding this comment

lgirdwood commented Mar 20, 2020

jajanusz commented Mar 25, 2020

lgirdwood commented Mar 26, 2020

dbaluta commented Feb 13, 2020 •

edited

Loading

jajanusz commented Feb 13, 2020 •

edited

Loading