Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add addc, subc and mulc functions to I128 and U128 #2645

Merged
merged 4 commits into from
May 9, 2018
Merged

Conversation

mfelsche
Copy link
Contributor

These methods are pretty slow on non native128 machines as we need to manually check for overflow.

fixes #2409

@mfelsche mfelsche added do not merge This PR should not be merged at this time changelog - added Automatically add "Added" CHANGELOG entry on merge labels Apr 11, 2018
@mfelsche mfelsche requested review from Praetonus and jemc April 11, 2018 12:31
@mfelsche
Copy link
Contributor Author

On my linux machine with llvm 5 i do get a missing symbol __muloti4 during linking.
It seems it considers my machine as native128 although it isn't?

can anyone reproduce?

@@ -26,7 +26,7 @@ PONY_EXTERN_C_BEGIN

/** Definition of a hash map entry for uintptr_t.
*/
typedef struct hashmap_entry_t
Copy link
Member

Choose a reason for hiding this comment

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

I think this was a stray erroneous change from a misrouted keyboard press into my editor. It should probably be removed.

@jemc
Copy link
Member

jemc commented Apr 11, 2018

Seems like we also want to add these methods to the _SignedInteger and _UnsignedInteger traits?

@jemc
Copy link
Member

jemc commented Apr 11, 2018

Yes, I see the same, but only on I128.mulc. If I remove the native version of that method, but keep everything else (including the native version of U128.mulc), everything works fine.

Is it possible that llvm.umul.with.overflow.i128 exists, but llvm.smul.with.overflow.i128 doesn't?

@mfelsche
Copy link
Contributor Author

mfelsche commented Apr 11, 2018

It seems there is a bug for that: https://bugs.llvm.org/show_bug.cgi?id=16404
The suggestion seems to be to link compiler-rt. I am gonna investigate. Or we just remove the llvm.smul.with.overflow.i128 and use the plain slow version only.

@jemc why not putting the functions into the Integer trait?

@mfelsche mfelsche changed the title Add addc, subs and mulc functions to I128 and U128 Add addc, subc and mulc functions to I128 and U128 Apr 11, 2018
@jemc
Copy link
Member

jemc commented Apr 11, 2018

why not putting the functions into the Integer trait?

If it works in the type system, that sounds fine to me.

@mfelsche
Copy link
Contributor Author

I put the functions into the Integer trait. This makes it easier to write generic code on any integer using addc, subc and mulc but it comes with the burden that custom Integer implementations also need an implementation for these functions. But i would consider extending Integer a rather rare use case and would favor the generic one.

@mfelsche
Copy link
Contributor Author

I did a small microbenchmark on my computer using ponybench against a I64 version of my __muloti4 port.

The code is in this gist: https://gist.github.com/52c8fb37b5595c1343e09045ab02761e (it actually doesn't run in the playground as it needs pony master for the new improved ponybench).

Here are some results:

Benchmark                                   mean            median   deviation  iterations
mulc.i64.llvm                           30481 ns          30276 ns      ±5.05%        5000
mulc.i64.manual                         29814 ns          29382 ns      ±3.54%        5000

mulc.i64.llvm                           29509 ns          28963 ns      ±4.11%        5000
mulc.i64.manual                         29345 ns          28866 ns      ±3.83%        5000

mulc.i64.llvm                           29068 ns          28840 ns      ±1.53%        5000
mulc.i64.manual                         31462 ns          29870 ns     ±14.08%        5000

mulc.i64.llvm                           29256 ns          29046 ns      ±1.63%        5000
mulc.i64.manual                         29109 ns          28832 ns      ±1.82%        5000

mulc.i64.llvm                           32610 ns          29894 ns     ±22.46%        5000
mulc.i64.manual                         29427 ns          28854 ns      ±4.24%        5000

# runs with --noadjust
Benchmark                                   mean            median   deviation  iterations
Benchmark Overhead                         20 ns             20 ns      ±1.24%    10000000
mulc.i64.llvm                           30673 ns          30485 ns      ±5.01%        5000
Benchmark Overhead                         21 ns             20 ns      ±5.36%    10000000
mulc.i64.manual                         32343 ns          32502 ns      ±6.83%        5000

Benchmark Overhead                         20 ns             20 ns      ±2.72%     5000000
mulc.i64.llvm                           31723 ns          31537 ns      ±4.22%        5000
Benchmark Overhead                         21 ns             20 ns      ±3.67%    10000000
mulc.i64.manual                         30275 ns          30121 ns      ±2.45%        5000

@mfelsche mfelsche removed the do not merge This PR should not be merged at this time label Apr 13, 2018
@mfelsche
Copy link
Contributor Author

mfelsche commented May 6, 2018

I would love to merge this, but i am hesitating without having anyone else take a look into it. Maybe @jemc or @Praetonus ?

@mfelsche mfelsche merged commit 9852d79 into master May 9, 2018
@mfelsche mfelsche deleted the safeops128 branch May 9, 2018 19:38
ponylang-main added a commit that referenced this pull request May 9, 2018
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
* WIP

* Add addc, subc and mulc functions to I128 and U128

* port __muloti4 to pony and use it in I128.mulc

* add addc, subc and mulc method to the Integer trait
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addc and subc functions not implemented for U128 and I128
2 participants