Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

If the wrap around does not work anyway, you can do the following and have it also work for all zeros and all ones. At least I think so, this is a bit ad hoc without putting too much thought into it.

  edges = x & (x ^ (x >> 1))
  edge  = edges & -edges
  mask  = edge | (edge << 1)
  
  x ^= mask
If you are willing to accept a conditional expression, this will make the wrap around work, I currently have no good idea how to make it work without a condition.

  x = (mask == 0) ? x <<< popcount(x) : x ^ mask
The idea is as follows. To increase the number, you have to toggle some bit from zero to one, and to make the increase as small as possible, it should be as far to the right as possible. But to keep the number of ones the same, you also have to toggle one bit from one to zero. This one must not be to the left of the bit you toggle from zero to one or the number would decrease, otherwise it should be as far to the left as possible to make the number as small as possible. In essence this means that you have to find the rightmost occurrence of 01 and turn it into 10.

x ^ (x >> 1) finds all the 01 and 10 edges, edges = x & (x ^ (x >> 1)) finds only the 01 edges. edge = edges & -edges clears all but the rightmost set bit - the two's complement first inverts all bits, then adding one turns all the ones on the right into zeros until reaching the rightmost zero which gets turned into a one - leaving use with one set bit indicating the rightmost 01 edge. mask = edge | (edge << 1) duplicates that bit and this is then used to flip the corresponding two bits with x ^= mask.

Eventually all set bits will end up on the left and there will be no 01 edge leaving x stuck in this state.



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.


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


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.


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.


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...


Fortunately my encounters with C and C++ have been rare and brief.


> I currently have no good idea how to make it work without a condition.

Would this work? Or not worth computing both values each time?

    x = ((mask == 0) * x <<< popcount(x)) & ((mask != 0) * x ^ mask)


I have no idea if this would be worth doing. One could also use a conditional move like CMOV on x86 if available on the target architecture to avoid a jump. On the one hand I have the gut feeling that there should be a nice trick to do this, on the other hand this seems to either require a population count dependent shift or rotation or reversing the word, which also has no really simple trick as far as I know and is usually done with a logarithmic number of shifts.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: