Skip to content

BUG: shift operator cycles, fixes #2449 - #7473

Closed
jaimefrio wants to merge 1 commit into
numpy:masterfrom
jaimefrio:bit_shifts
Closed

jaimefrio wants to merge 1 commit into
numpy:masterfrom
jaimefrio:bit_shifts

Conversation

@jaimefrio

@jaimefrio jaimefrio commented Mar 27, 2016 •

Copy link
Copy Markdown
Member


#define LEFT_SHIFT_OP \
do { \
if (NPY_LIKELY(in2 < sizeof(@type@) * 8)) { \

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.

I think there are number of bits macros defined somewhere... yep, in npy_common.h. The have the form NPY_BITSOF_LONGLONG etc.

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.

Probably already included here.

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.

Please, kill the macro ;)

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.

Does this actually buy speed over ...? ... : ...?

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.

A cleaner version modulo signedness might be

#define LEFT_SHIFT = do {*out = NPY_LIKELY(in2 < NPY_BITSOF_@TYPE@) ? in1 << in2 : 0} while(0)

Although I don't think we actually need the do ... while(0) bit here.

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.

Macro removed in #13739. Kept the if statements because I think they're clearer. Did not use NPY_BITS_OF_LONG and friends because they don't exist for NPY_BITS_OF_ULONG etc.

@charris

charris commented Mar 29, 2016

Copy link
Copy Markdown
Member

Might want to also check the scalar math in scalarmath.c.src, seems to go as @name@_{l,r}shift. Would need separate tests also, unfortunately.

{
if (IS_BINARY_REDUCE) {
BINARY_REDUCE_LOOP(@type@) {
@type@ ip2_val = *(@type@ *)ip2;

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.

Could remove the reduce specialization, I suspect reduce using the shift operators would be rare and BINARY_LOOP_FAST should have adequate performance.

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.

Done in #13739

@charris

charris commented Apr 16, 2016

Copy link
Copy Markdown
Member

Be good to finish this up for 1.12.

@charris charris added this to the 1.12.0 release milestone Apr 16, 2016
@charris

charris commented Sep 7, 2016

Copy link
Copy Markdown
Member

@jaimefrio How is your time looking?

@jaimefrio

Copy link
Copy Markdown
Member Author

I'll take a look at it during this week/weekend.

@charris

charris commented Sep 19, 2016

Copy link
Copy Markdown
Member

@jaimefrio Waiting... If you don't have the time let me know and I'll try to fix it up.

@homu

homu commented Sep 25, 2016

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #7980) made this pull request unmergeable. Please resolve the merge conflicts.

@charris

charris commented Oct 20, 2016

Copy link
Copy Markdown
Member

I wanted to get this into 1.12 but don't have the time at the moment, so pushing off to 1.13.

@charris charris modified the milestones: 1.14.0 release, 1.13.0 release May 3, 2017
@charris charris modified the milestones: 1.14.0 release, 1.15.0 release Nov 11, 2017
@charris

charris commented Nov 11, 2017

Copy link
Copy Markdown
Member

Pushed off to 1.15.

Comment thread numpy/core/src/umath/loops.c.src Resolved
@mattip

mattip commented Apr 26, 2018

Copy link
Copy Markdown
Member

@jaimefrio - ping?

@charris

charris commented May 25, 2018

Copy link
Copy Markdown
Member

Pushing off again.

@charris charris modified the milestones: 1.16.0 release, 1.17.0 release Nov 17, 2018
@charris

charris commented Nov 17, 2018

Copy link
Copy Markdown
Member

Pushed off again.

@eric-wieser

Copy link
Copy Markdown
Member

I don't have permissions to push fixups to this branch, so I fixed the conflicts and addressed comments in gh-13739

@charris charris removed this from the 1.17.0 release milestone Jun 14, 2019
@charris

charris commented Jun 14, 2019

Copy link
Copy Markdown
Member

@eric-wieser OK, closing this.

@charris charris closed this Jun 14, 2019
eric-wieser added a commit to eric-wieser/numpy that referenced this pull request Sep 13, 2019
This:
* Inlines the macros in loops.c.src
* Replaces 8 with `CHAR_BIT `. The `NPY_SIZEOF_*` macros are not used here because it's too much work to apply them to the signed types, and they expand to the same thing anyway.
* Removes the reduce loop specializations which likely no one cares about
* Uses pytest.mark.parametrize to shorten the test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants