RFR: 8230203: Replace markWord enums with uintptr_t constants

[David Holmes]

On 28/08/2019 12:05 pm, David Holmes wrote:
> Hi Stefan,
> 
> On 28/08/2019 6:06 am, Stefan Karlsson wrote:
>> Hi Kim,
>>
>> I think your comments makes sense. I have an updated webrev that 
>> changes some of the types:
>>
>>   https://cr.openjdk.java.net/~stefank/8230203/webrev.02.delta/
>>   https://cr.openjdk.java.net/~stefank/8230203/webrev.02/
> 
> I agree that is much clearer now. "bits" and "shifts" are just numbers. 
> masks and comparison-constants should be the same type as the 
> variable(s) they will be applied to.
> 
> In markWord.hpp:
> 
> 334     assert(age <= max_age, "age too large");
> 
> This assert is a tautology as age and max_age are the same uint type. (I 
> expect some compilers may point this out.)

Ignore that idiotic comment. <sigh.

David

> 
> Thanks,
> David
> -----
> 
>> The new patch compiles on Linux x64 and not yet tested. I'll run it 
>> through testing if you think this is the right change.
>>
>> Thanks,
>> StefanK
>>
>> On 2019-08-27 20:04, Kim Barrett wrote:
>>>> On Aug 27, 2019, at 3:32 AM, Stefan Karlsson 
>>>> <stefan.karlsson at oracle.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Please review this patch to convert the enum constants in markWord 
>>>> into static const unintptr_t constants.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8230203/webrev.01/
>>>> https://bugs.openjdk.java.net/browse/JDK-8230203
>>>>
>>>> This is a follow-up RFE from the review of JDK-8229258, where a 
>>>> number of unnecessary casts were pointed out.
>>>>
>>>> There was also a recent bug in this area, JDK-8221725, where Thomas 
>>>> points out the problem of relying on the bit width of the enum values.
>>>>
>>>> This patch:
>>>> 1) changes all constants to have the same type as the 
>>>> markWord::_value field.
>>>> 2) Removes unnecessary uintptr_t casts
>>>> 3) Removes uintx casts, because they are typedefed to be uintptr_t
>>>> 4) Adds explicit casts to (int) when values are negated.
>>>> 5) Try to only use uintptr_t inside markWord.
>>>> 6) Leaves the use of intptr_t for parameters and return types of the 
>>>> different hash functions.
>>>>
>>>> Tested with tier1-3
>>>>
>>>> Thanks,
>>>> StefanK
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/hotspot/share/oops/markWord.hpp
>>>   134   static const uintptr_t age_bits                 = 4;
>>> ...
>>>
>>> and
>>>   144   static const uintptr_t lock_shift               = 0;
>>> ...
>>>
>>> The bit counts and shift counts are used as rhs in shift expressions
>>> and such.  I think they should be int or uint rather than uintptr_t.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/hotspot/share/oops/markWord.hpp
>>>   334     assert((uintptr_t)age <= max_age, "age too large");
>>>
>>> I think the cast here is unnecessary and not really helpful.
>>>
>>> Unfortunately, the cast in the immediately following line:
>>>   335     assert((uintptr_t)bias_epoch <= max_bias_epoch, "bias epoch 
>>> too large");
>>>
>>> probably is needed to avoid -Wsign-compare warnings.  (Though
>>> JDK-8230118 suggests we might not be getting -Wsign-compare warnings
>>> when one might think we ought.)
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> src/hotspot/share/oops/markWord.hpp
>>>   409     assert((size & ~size_mask) == 0, "shouldn't overflow size 
>>> field");
>>>   410     return markWord((cms_free_prototype().value() & 
>>> ~size_mask_in_place) |
>>>   411                     ((size & size_mask) << size_shift));
>>>
>>> I wonder why 411 is applying size_mask after the 409 assert?
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>> I think there were 3 casts to int added:
>>>
>>> src/hotspot/share/opto/macro.cpp
>>> 2229 (~(int)markWord::age_mask_in_place), 0);
>>>
>>> src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
>>> 3003   addi(current_header, current_header, 
>>> -(int)markWord::monitor_value); // monitor
>>>
>>> src/hotspot/cpu/aarch64/aarch64.ad
>>> 3619     __ add(tmp, tmp, -(int)markWord::monitor_value); // monitor
>>>
>>> Consider preceeding these with STATIC_ASSERTs that the value being
>>> cast is <= INT_MAX.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>