Allow negative bit-shift counts #14791

eschnett · 2016-01-25T18:09:58Z

This patch concerts the bit shift operators << >> >>>.

Convert signed bit-shift counts explicitly to unsigned, reporting InexactErrors for negative counts.
Introduce versions that take unsigned arguments, and which thus do not require any tests.
Introduce new functions bitshift and unsigned_bitshift that (similar to Matlab) shift left or right for positive and negative counts, respectively.
Add documentation.
Add tests.

tkelman · 2016-01-25T19:03:19Z

unsigned_bitshift is not a name we should export. What's the simplest description of the difference between x >> -n and x >>> -n here?

eschnett · 2016-01-25T19:49:38Z

Should we even have unsigned_bitshift? How commonly is >>> used? This function is not necessary; one can always convert to the respective unsigned type instead.

In machine language, this would be called a "logical shift".

simonbyrne · 2016-01-25T19:50:44Z

We could use <> or <<> as operators?

tkelman · 2016-01-25T19:50:49Z

Leaving it out sounds appropriate.

simonbyrne · 2016-01-25T19:51:40Z

>>> appears 109 times in Base, >> 372.

eschnett · 2016-02-11T02:56:53Z

Removed unsigned_bitshift, rebased.

tkelman · 2016-02-11T03:05:53Z

LGTM. @StefanKarpinski what do you think?

tkelman · 2016-02-11T05:41:58Z

IIUC this is breaking, so it could use a mention in NEWS.md.

StefanKarpinski · 2016-02-11T11:35:04Z

I'm not sure. I'm somewhat more inclined to define <<, >> and >>> to work "correctly" with negative shift values – and not introduce the bitshift function. The generated code is kind of long, but potentially throwing an exception is also a total performance disaster, so anyone who's not using shift values that the compiler can reason about who cares about performance would need to do additional work anyway.

StefanKarpinski · 2016-02-11T11:35:23Z

@JeffBezanson, thoughts?

eschnett · 2016-02-19T15:34:54Z

Ping

eschnett · 2016-02-26T12:41:34Z

@StefanKarpinski: It seems @JeffBezanson doesn't have input on this. What's your verdict?

StefanKarpinski · 2016-02-26T13:51:23Z

Rather than introducing a new bitshift function, let's make negative shifts work for existing shift ops:

a  << -b == a >> b
a  >> -b == a << b
a >>> -b == a << b

I suspect that in this case, using a branch will be better than using a select instruction (ifelse) since the branch will tend to be very predictable. Otherwise keep the rest of this change – i.e. making the unsigned version fundamental, so that you can avoid the branch entirely by using an unsigned shift value.

@eschnett, do you mind modifying this PR this way (or making a new one)?

simonbyrne · 2016-02-26T13:59:02Z

Rather than introducing a new bitshift function, let's make negative shifts work for existing shift ops:

+1

carnaval · 2016-02-26T14:28:45Z

as long as constant shifts get folded correctly I think that's fine

eschnett · 2016-02-26T15:47:07Z

I think might be worth a benchmark.

simonbyrne · 2016-02-27T09:59:58Z

base/gmp.jl

@@ -346,23 +346,27 @@ for (fJ, fC) in ((:-, :neg), (:~, :com))
    end
 end

-function <<(x::BigInt, c::Int)
-    c < 0 && throw(DomainError())
+function <<(x::BigInt, c::UInt)


Shouldn't this be Culong?

Making this Culong would force every caller to explicitly convert to Culong.

Ah, I misread it (I thought it was Cuint).

What if Culong != UInt (as I think it is on Windows 64-bit): won't the implicit conversion throw an error for typemax(UInt)?

Possibly; I don't know whether ccall checks. 64-bit Linux definitely has this problem. That problem was there before, though.

I don't know a good solution -- shifting a BigInt left by a large number should work "in principle", but will in practice run out of memory. So an error message is probably the best we can do anyway.

That's probably true.

eschnett · 2016-03-01T15:27:35Z

Are there any further comments or requests? I'm ready to squash and merge.

eschnett · 2016-03-03T02:07:13Z

Rebased and squashed, Travis and Appveyor are green. Ready to go?

tkelman · 2016-03-04T05:32:38Z

base/operators.jl

-where `n >= 0`, filling with `0`s. This is equivalent to `x * 2^n`.
+Left bit shift operator, `x << n`. For `n >= 0`, the result is `x` shifted left
+by `n` bits, filling with `0`s. This is equivalent to `x * 2^n`. For `n < 0`,
+this is equivalent to `x >> -n`.


needs a make docs to update the rst contents

Done. Anything else I missed?

I think we're good.

- Introduce methods that take unsigned shift counts - Handle signed shift counts by dispatching to the respective unsigned shift counts, shifting in the opposite direction for negative counts - Update documentation - Add tests Closes JuliaLang#14516.

Allow negative bit-shift counts

tkelman · 2016-03-05T10:15:31Z

This seems to have introduced several ambiguity warnings during bootstrap.

eschnett · 2016-03-05T14:43:52Z

I will check.

eschnett · 2016-03-05T15:22:01Z

This is now PR #15368.

tkelman · 2016-08-19T18:08:11Z

IIUC this is breaking, so it could use a mention in NEWS.md.

This was never addressed here, was it?

eschnett · 2016-08-19T19:07:41Z

Good catch.

eschnett · 2016-08-22T18:52:28Z

@tkelman In what sense in this breaking? The new functionality is a strict superset of the old one. Do you refer to how one now defines bit-shift operations for user-defined types? Or do you refer to the performance difference between signed and unsigned shift counts?

tkelman · 2016-08-22T18:59:13Z

None of those - weren't we previously doing the Perl behavior for #14516 (comment), so after this PR negative shifts now give different results?

eschnett · 2016-08-22T19:12:40Z

Very good; yes, I understand now. I'll add the NEW.md entry.

eschnett · 2016-08-22T19:25:30Z

See ##18188.

simonbyrne mentioned this pull request Jan 25, 2016

Expand docs for bitshift operators #14790

Merged

eschnett force-pushed the eschnett/negative-shifts branch 2 times, most recently from e462f12 to 60d4821 Compare February 11, 2016 00:04

eschnett added potential benchmark Could make a good benchmark in BaseBenchmarks and removed potential benchmark Could make a good benchmark in BaseBenchmarks labels Feb 26, 2016

eschnett force-pushed the eschnett/negative-shifts branch from 60d4821 to 9a0e2b6 Compare February 26, 2016 16:43

eschnett changed the title ~~Forbid negative bit-shift counts~~ Allow negative bit-shift counts Feb 26, 2016

simonbyrne reviewed Feb 27, 2016
View reviewed changes

eschnett force-pushed the eschnett/negative-shifts branch from ee99029 to 78a2d09 Compare February 27, 2016 15:29

eschnett force-pushed the eschnett/negative-shifts branch from 6072d28 to e3d5fda Compare March 2, 2016 17:54

tkelman reviewed Mar 4, 2016
View reviewed changes

eschnett force-pushed the eschnett/negative-shifts branch from e3d5fda to 5a9717b Compare March 4, 2016 14:27

eschnett added a commit that referenced this pull request Mar 5, 2016

Merge pull request #14791 from eschnett/eschnett/negative-shifts

a505b61

Allow negative bit-shift counts

eschnett merged commit a505b61 into JuliaLang:master Mar 5, 2016

eschnett deleted the eschnett/negative-shifts branch March 6, 2016 14:01

tkelman added the needs news A NEWS entry is required for this change label Aug 19, 2016

tkelman removed the needs news A NEWS entry is required for this change label Aug 22, 2016

Allow negative bit-shift counts #14791

Allow negative bit-shift counts #14791

Conversation

eschnett commented Jan 25, 2016

tkelman commented Jan 25, 2016

eschnett commented Jan 25, 2016

simonbyrne commented Jan 25, 2016

tkelman commented Jan 25, 2016

simonbyrne commented Jan 25, 2016

eschnett commented Feb 11, 2016

tkelman commented Feb 11, 2016

tkelman commented Feb 11, 2016

StefanKarpinski commented Feb 11, 2016

StefanKarpinski commented Feb 11, 2016

eschnett commented Feb 19, 2016

eschnett commented Feb 26, 2016

StefanKarpinski commented Feb 26, 2016

simonbyrne commented Feb 26, 2016

carnaval commented Feb 26, 2016

eschnett commented Feb 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschnett commented Mar 1, 2016

eschnett commented Mar 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Mar 5, 2016

eschnett commented Mar 5, 2016

eschnett commented Mar 5, 2016

tkelman commented Aug 19, 2016

eschnett commented Aug 19, 2016

eschnett commented Aug 22, 2016

tkelman commented Aug 22, 2016

eschnett commented Aug 22, 2016

eschnett commented Aug 22, 2016