RFR: 8257069: C2: Clarify and sanity test RegMask/RegMaskIterator [v2]

[John R Rose]

On Wed, 25 Nov 2020 21:18:09 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Simplify and use type declared constants, minor cleanups
>
> Good.

This is not a review comment, because you didn't change the algorithm, but:  That code is hard to follow, equally before and after.  There is a lot of delicate "off-by-one" logic in it.  Since it Just Works ™ we might choose to ignore this issue, but it is tech. debt that must be paid whenever we might try to improve the code in the future.

More detail, FWIW:

The treatment of _current_word as a state variable is very confusing, because it is a mish-mash of two styles of a bitmask state.  Style one is “keep the bits in place but clear them one by one”.  Style two is “shift out the bit positions one by one.”  The style here is “shift out all the bits except the current one, but clear it for good measure”.  That’s IMO hard to read and reason about.  If I were writing it from scratch, I would prefer the first style, where there is no shifting of _current_word, just bit-by-bit clearing to zero.  This does require a second _reg-like state variable to restart the bit-search, or else a constant-time FFS operation.  Failing that, I’d prefer the second style, shifting out not only the zero positions, but the next one position.  But that’s probably almost as liable to O-B-O errors, since you have to shift >> (next_bit+1), and increment _reg by a similar amount.  Anyway, surely there’s a way to make code like this more self-explan
 atory.  Comments would be a kind service to future readers.

As a variation of the "whiteboard effect" (which is what happens when you explain something to a colleague and at the same time get better understanding of what you are explaining), adding well-written comments to code like this often suggests a better way to formulate the code itself.  At least, that's been my experience.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1437