RFR: 8230203: Replace markWord enums with uintptr_t constants

[David Holmes]

Hi Stefan,

On 27/08/2019 5:32 pm, Stefan Karlsson 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

Overall this seems like a good cleanup, but the introduced "int" casts 
now jump out at me. In:

  src/hotspot/cpu/aarch64/aarch64.ad
  src/hotspot/cpu/arm/macroAssembler_arm.cpp
  src/hotspot/cpu/ppc/macroAssembler_ppc.cpp

it would seem to me that all the "add -(int)markWord::monitor_value" 
would be cleaner/clearer as: "sub markWord::monitor_value". But that is 
a bit out of scope for your change - it just jumped out at me. I expect 
add is more efficient than sub.

In  src/hotspot/share/opto/macro.cpp I don't understand why this int 
cast is needed:

       Node* not_biased_ctrl =  opt_bits_test(ctrl, region, 3, x_node,
! 
(~(int)markWord::age_mask_in_place), 0);

I'm unclear if this is really

~(int)markWord::age_mask_in_place

or

(int)(~markWord::age_mask_in_place)

? If the latter then okay. If the former then I don't grok it. :)

Thanks,
David
-----

> 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