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

breaking: polish @clz() @ctz() @popcount() @bswap() @bitReverse() #2134

Closed
wants to merge 22 commits into from

Conversation

shawnl
Copy link
Contributor

@shawnl shawnl commented Mar 29, 2019

Closes: #2119

breaking: @bitreverse -> @bitreverse, @bswap -> @bswap

Closes: #2120

Comes with tests

OK. This has changed quite a bit since it was opened. It gained @fshl and @fshr , and then I added @memMove and merged a differn't pull request so I could flesh out @memMove. I am working on #767

@andrewrk
Copy link
Member

Why are the new files added?

@shawnl
Copy link
Contributor Author

shawnl commented Mar 29, 2019

Why are the new files added?

Whoops, thanks for spotting that.

@andrewrk andrewrk self-requested a review March 29, 2019 16:35
@shawnl shawnl force-pushed the builtins-consistancy branch 7 times, most recently from 7e25928 to a401388 Compare April 5, 2019 02:28
@shawnl
Copy link
Contributor Author

shawnl commented Apr 7, 2019

I'm now working on exanding the vector features sufficiently to get this algorithm ported to zig https://github.com/cyb70289/utf8 using llvm vectors, rather than assembly intrinsics.

@shawnl shawnl force-pushed the builtins-consistancy branch from f1c765d to 157ade9 Compare April 7, 2019 15:22
@shawnl shawnl force-pushed the builtins-consistancy branch from 3e43f36 to c005271 Compare April 8, 2019 15:21
no safety checks

no 0-width type support
@shawnl shawnl force-pushed the builtins-consistancy branch from c005271 to 1430867 Compare April 8, 2019 20:13
shawnl added 3 commits April 9, 2019 19:39
Also, change the interface to accept an appropiately ranged type.
While this is technically breaking, no users used this useless feature.
var neg32: i32 = -16773785;
expect(@bitreverse(i32, -16773785) == @bitreverse(i32, neg32));
var neg40: i40 = minInt(i40) + 12345;
expect(@bitreverse(i40, minInt(i40) + 12345) == @bitreverse(i40, neg40));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because they are tautologies. They are not testing bitreverse.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the time that code was written, I'm not sure about now, the comment at the top of the block explained correctly what was happening. The LHS values were calculated by comptime code in the Zig compiler while the RHS values are calculated on the LLVM side. These tests were used to make sure the two match.

Copy link
Member

Choose a reason for hiding this comment

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

@vegecode is correct, and it's still true. @bitreverse(i32, -16773785) exercises comptime evaluation; @bitreverse(i32, neg32) with neg32 as a var exercises runtime evaluation.

@shawnl shawnl force-pushed the builtins-consistancy branch from 1292aa0 to e90f46e Compare April 10, 2019 18:13
@andrewrk
Copy link
Member

This is too much. I can't keep up with this ever-growing pull request. @shawnl I need you to make one pull request at a time, which only tackles one thing.

@andrewrk andrewrk closed this Apr 10, 2019
@andrewrk andrewrk mentioned this pull request Apr 11, 2019
Sign up for free to 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.

4 participants