RFR: 8186166: Generalize Atomic::cmpxchg with templates

[Kim Barrett]

> On Aug 15, 2017, at 10:22 AM, Roman Kennke <rkennke at redhat.com> wrote:
> 
> Am 14.08.2017 um 11:56 schrieb Kim Barrett:
>>> On Aug 14, 2017, at 10:28 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>> 
>>> I still don't understand why we want to allow for different types for
>>> the new and expected values and the memory location (T, D and U). We
>>> haven't allowed for this before, why are we doing it now?
>> Different types are only permitted in the pointer value case,
>> e.g. cmpxchg(T*, D* volatile*, U*).
>> 
>> The compare_value type is permitted to differ from the destination's
>> value type only in cv-qualifiers.  Consider a member variable "var" of
>> type int*.
>> 
>>  const int* old_value = var;
>>  ... compute new_value ...
>>  cmpxchg(new_value, &var, old_value);
>> 
>> It is perfectly reasonable and appropriate to const-qualify
>> old_value's type in that way.  But that would require a source level
>> cast if cmpxchg required an exact type match for *dest and
>> compare_value.
>> 
>> We permit the exchange_value type to be less cv-qualified than the
>> *dest type.  We also permit derived to base conversions.  Both of
>> those are natural and type-safe, and may eliminate the need for some
>> source level casts.
>> 
>>> We haven't allowed for this before, why are we doing it now?
>> Because of the complete type-discarding done by cmpxchg_ptr, I suggest
>> we have no clue what our current code might or might not be doing.
>> 
> I don't see that anywhere. The only situations that I could find are a
> few places where dest is cast to (volatile foo*), which makes me
> question whether that dest field should be marked volatile in the first
> place. I see no patterns as you described above involving any const
> casts. And I am absolutely against introducing some machinery that is
> not actually used. If it's not used, it's dead, and only increases debt
> rather than decrease it. (Note: the situation would be different if it
> were a public API. but it isn't)

The API affects the surrounding using code.  There being no uses of
more cv-qualifed compare_value than the destination type shouldn't be
surprising, since the present API doesn't allow *any* cv-qualifiers
for the compare_value.  But I suggest that's a bug in the present API,
much like the bug that Atomic::load didn't const-qualify the source
(which I fixed not so long ago).

Hotspot code is missing a lot of const qualifiers.  I ran into this
one myself at some point, with exactly the use-case I described in
earlier mail, and was quite annoyed that the API inhibited writing
obviously correct and idiomatic code.

For the exchange_value, I just encountered a case where the
cv-qualifiers of that new value should be expected to differ from the
destination.  In the published webrev is a change to
BitMap::init_pop_count_table() to use the new conditional_store_ptr
instead of cmpxchg_ptr with a NULL compare_value.  What I didn't
notice at the time is that the type of _pop_count_table should be
const BitMap::idx_t*, but was missing the const qualifier.  I've fixed
that, and because of the implicit conversion support, that just
worked.  If the new value type had to match that const qualified type,
there would need to be a conversion of some kind added to the caller.
The new value is a just constructed and explicitly filled in array, so
the variable holding it clearly can't have that const qualifier in its
type.  An API that requires such an explicit conversion in the caller
in a case like this is simply broken, in my opinion.