[stdlib] Rationalize bitshifting in protocols #11044
Conversation
Only FixedWidthInteger, not BinaryInteger, should have masking shifts. BinaryInteger should have a non-masking shift requirement. Removed some dead code.
|
@swift-ci Please test |
|
@swift-ci Please benchmark |
|
@swift-ci Please test source compatibility |
|
/cc @natecook1000 |
|
Build failed |
|
Source compatibility just registers a UPASS on Chatto |
Build comment file:Optimized (O)Regression (4)
Improvement (3)
No Changes (314)
Unoptimized (Onone)Regression (5)
Improvement (2)
No Changes (314)
Hardware Overview |
|
Since masking shifts can more efficient than non-masking ones, and since it is not possible to implement the same generic method twice, once with an additional constraint, wouldn't there be performance implications to redesigning in this way? IMO, it is fine to consider masking shift as the most primitive operation; for a binary integer of arbitrary width, it should simply always be the same operation as the non-masking shift. This can be achieved by redefining masking shift semantics without any change in the syntax or protocol design. |
|
@xwu good questions. Fortunately I think I have answers. W.r.t. performance implications, this only comes up if:
I'm not even sure there is a use case like this, but if a use case does exist we should figure out a way to support it. The following might be a useful component of that solution, though admittedly it only generates good code for the fixed-width integers and thus suggests extension BinaryInteger {
static var isFixedWidth: Bool {
return isSigned
? Magnitude(extendingOrTruncating: ~Self()) != ~Self()
: (~Self() << 1) < ~Self()
}
}With regard to defining the semantics as you suggest, I personally didn't see any way to specify them so they made sense, that didn't get into a horrible tangle. Try to write out the formal semantic requirements of |
|
I've thought about the By this I mean that This is all to say that As to the semantics of
Note that this definition requires |
|
Thanks, @xwu—I think I'm mostly with you. Although within the current system we can't say a It's helpful for me to make a distinction between what a masking shift does and what it's for. What it does is right in the name—it masks the RHS value to be in the range of That is to say, it isn't designed so we can write code like this: let x: UInt8 = 2
let y = x &<< 10 // 10 & 0x7 == 2, so y == 2 << 2 == 8Instead, it's so we can write code like this with the best possible performance, while still having defined behavior when LHS is outside the bounds that would be caught by a smart shift: let x: UInt8 = 2
let y = x &<< 2 // or really shifted by some value we statically know is in 0..<8I still don't think it's clear that there are any cases where the criteria Dave set forth above are met, so to me the "standard" shift operators make more sense at the |
|
@xwu a few thoughts I'd like to add to what Nate said. I agree strongly that But I don't think I want to handle things this way, because of what it does to the semantic description of |
|
@natecook1000 @dabrahams Very good points. Nate, you bring up an interesting question about masking shift by a negative value. In playing with implementing a bignum type, I had to figure out what to do about exactly that under the current semantics. It is not a useful operation; the semantics would imply that the result should be infinitely large, since, after masking, any negative value would be infinitely large. Absent a representation for infinity, this should be an error. This supports the notion that As to what useful algorithms I'm interested in, the answer is simply: For any arbitrary integer value |
|
@xwu if what you're really interested in is 0 ≤ |
|
@dabrahams Right, that's not quite what I'm after. The upperbound-ness of |
|
Looking a little further, I'm not convinced that |
|
@swift-ci Please Test Source Compatibility |
This expression became too complex due to the changes introduced in swiftlang/swift#11044
|
apple/swift-corelibs-foundation#1124 |
|
I went spelunking in the SE-104 history to see how we arrived at this situation:
@dabrahams Do you mean getting rid of the instance-level |
|
@natecook1000 @dabrahams Agree that the current semantics of instance-level |
|
@xwu because |
|
@dabrahams It's turtles all the way down: * The most sensible semantics for bitwise operations on arbitrary-width integers (an infinite sequence of bits, etc.) would have the hypothetical leading zero bit count and bit width both as infinity. What I'm getting at is we need some way to spell the "bit scan reverse" (BSR) intrinsic (of the absolute value); the integer binary logarithm of the magnitude, if you will. |
This expression became too complex due to the changes introduced in swiftlang/swift#11044
This expression became too complex due to the changes introduced in swiftlang/swift#11044
|
apple/swift-corelibs-foundation#1124 |
|
on Fri Jul 21 2017, Xiaodi Wu <notifications-AT-github.com> wrote:
@dabrahams It's turtles all the way down: `leadingZeroBitCount` is a
property of fixed-width integers only (unlike `trailingZeroBitCount`).
It's also worth pointing out that words.count gives us all the useful
information currently available from bitWidth.
|
|
In many places instance |
|
Perhaps we can underscore it for now... in order to discourage usages, and see what to do with it at a later date. |
|
on Mon Jul 24 2017, Maxim Moiseev <notifications-AT-github.com> wrote:
In many places instance `bitWidth` is used in the branches in generic
code over `BinaryInteger` with the hope that optimizer will inline a
constant there and eliminate those branches completely in a fully
specialized code. Even though it's not a good enough reason to design
an API based on the capabilities of the compiler, replacing instance
`bitWidth` with `words.count` almost certainly will result in
regressions.
I can't imagine why, if Words is CollectionOfOne(UInt) in almost every
case.
|
|
@dabrahams no reason to not try it then. |
|
Huh. |
|
@moiseev With the current compiler, it may be possible to add |
|
I took a second look on the latest master and it seems This enables the use of |
|
on Tue Jul 25 2017, Károly Lőrentey <notifications-AT-github.com> wrote:
@moiseev `Collection` conformance for `Words` requires compiler support for recursive
constraints. (It works with 03f3a0c.)
With the current compiler, it may be possible to add `_IndexableBase`
or even `_Indexable` conformance; unfortunately the `count` property
is in neither of those protocols.
We should just move the property requirement up in the protocol
hierarchy as high as we need it to be.
…--
-Dave
|
moiseev
changed the title
This requires at least core team, if not evolution, discussion.
Only FixedWidthInteger, not BinaryInteger, should have masking shifts.
BinaryInteger should have a non-masking shift requirement.
Removed some dead code.