3 ms·

THE ALGORITHM IS WRONG. But I can no longer edit it. 1110 should become 10011 but my »solution« yields 10110. Will have to rethink this.

danbruc 4y ago
And here is the fixed version, I hope. zeroOneEdges = x & (x ^ (x >> 1)) // This must be a signed arithmetic shift. rightmostZeroOneEdge = zeroOneEdges & -zeroOneEdges toggleMask = rightmostZeroOneEdge | (rightmostZeroOneEdge << 1) rightmostOne = x & -x shiftMask = rightmostZeroOneEdge - 1; onesToShift = x & shiftMask shiftedOnes = onesToShift / rightmostOne // This must be an unsigned division. x = ((x & ~shiftMask) ^ toggleMask) | shiftedOnes Now it wraps around without a conditional expression but just as the one from the library will divide by zero if called with zero, so either do not do this or add a check for zero and return zero in that case. It works for all ones. My first attempt was on the right track but also missing a step. Not only must the rightmost 01 edge be turned into 10 but also all the ones to the right of the 01 edge must be shifted back to the right, i.e. <rest>01<ones><zeros> must become <rest>10<zeros><ones>. This shift is done with the division and it is important that it is an unsigned division. It is also important that the 01 edge detection shift is a signed arithmetic shift. The algorithm from the library and my solution are quite similar and they might actually be identical with the one from the library being more optimized and shorter. I would not be surprised if the one from the library actually also works in principle but the implementation got a tiny detail about the signedness of one of the operations wrong, because that will result in the described issues, i.e. not working properly once the sign bit gets involved, either for all ones or during the wrap around. EDIT: I had a close look at the algorithm from the library and it is indeed broken - in order to work as intended, the shift in ones = (ones >> 2) / smallest would have to sometimes perform a logical shift and sometimes an arithmetical shift depending on the circumstances. This should be fixable. EDIT: Combining the ideas from both, this is my current best solution that handles everything including the wrap around properly, besides zero. rightmostOne = x & -x zeroOneEdges = x & (x ^ (x >> 1)) // This must be a signed arithmetic shift. rightmostZeroOneEdge = zeroOneEdges & -zeroOneEdges shiftedOnes = (x & (rightmostZeroOneEdge - 1)) / rightmostOne // This must be an unsigned division. x = (x + rightmostOne) | shiftedOnes
- anonymoushn 4y ago
  Super cool, I'll have to have closer look sometime. To handle zero, if you have ctz, you can replace the divide with two bitwise shifts. This may sometimes be faster than the divide even if you don't have ctz.
  - danbruc 4y ago
    Two shifts? I think ctz plus a single shift should do. shiftedOnes = (x & (rightmostZeroOneEdge - 1)) >> ctz(x) // This must be a logical shift. With all the fancy bit level instruction, this could probably be simplified in general, but I usually avoid them because I mostly see this as an exercise in making it work if you only have access to the basic arithmetic and logic operations and shifts.
    - anonymoushn 4y ago
      You need two shifts in C++ because shifting a uint32_t by 32 is UB. Every actual computer probably gives you 0 if you shift 0 right by a too-large amount though...
      - danbruc 4y ago
        Fortunately my encounters with C and C++ have been rare and brief.

danbruc 4y ago

And here is the fixed version, I hope. zeroOneEdges = x & (x ^ (x >> 1)) // This must be a signed arithmetic shift. rightmostZeroOneEdge = zeroOneEdges & -zeroOneEdges toggleMask = rightmostZeroOneEdge | (rightmostZeroOneEdge << 1) rightmostOne = x & -x shiftMask = rightmostZeroOneEdge - 1; onesToShift = x & shiftMask shiftedOnes = onesToShift / rightmostOne // This must be an unsigned division. x = ((x & ~shiftMask) ^ toggleMask) | shiftedOnes Now it wraps around without a conditional expression but just as the one from the library will divide by zero if called with zero, so either do not do this or add a check for zero and return zero in that case. It works for all ones. My first attempt was on the right track but also missing a step. Not only must the rightmost 01 edge be turned into 10 but also all the ones to the right of the 01 edge must be shifted back to the right, i.e. <rest>01<ones><zeros> must become <rest>10<zeros><ones>. This shift is done with the division and it is important that it is an unsigned division. It is also important that the 01 edge detection shift is a signed arithmetic shift. The algorithm from the library and my solution are quite similar and they might actually be identical with the one from the library being more optimized and shorter. I would not be surprised if the one from the library actually also works in principle but the implementation got a tiny detail about the signedness of one of the operations wrong, because that will result in the described issues, i.e. not working properly once the sign bit gets involved, either for all ones or during the wrap around. EDIT: I had a close look at the algorithm from the library and it is indeed broken - in order to work as intended, the shift in ones = (ones >> 2) / smallest would have to sometimes perform a logical shift and sometimes an arithmetical shift depending on the circumstances. This should be fixable. EDIT: Combining the ideas from both, this is my current best solution that handles everything including the wrap around properly, besides zero. rightmostOne = x & -x zeroOneEdges = x & (x ^ (x >> 1)) // This must be a signed arithmetic shift. rightmostZeroOneEdge = zeroOneEdges & -zeroOneEdges shiftedOnes = (x & (rightmostZeroOneEdge - 1)) / rightmostOne // This must be an unsigned division. x = (x + rightmostOne) | shiftedOnes

anonymoushn 4y ago
Super cool, I'll have to have closer look sometime. To handle zero, if you have ctz, you can replace the divide with two bitwise shifts. This may sometimes be faster than the divide even if you don't have ctz.
- danbruc 4y ago
  Two shifts? I think ctz plus a single shift should do. shiftedOnes = (x & (rightmostZeroOneEdge - 1)) >> ctz(x) // This must be a logical shift. With all the fancy bit level instruction, this could probably be simplified in general, but I usually avoid them because I mostly see this as an exercise in making it work if you only have access to the basic arithmetic and logic operations and shifts.
  - anonymoushn 4y ago
    You need two shifts in C++ because shifting a uint32_t by 32 is UB. Every actual computer probably gives you 0 if you shift 0 right by a too-large amount though...
    - danbruc 4y ago
      Fortunately my encounters with C and C++ have been rare and brief.