RFR: 8230203: Replace markWord enums with uintptr_t constants

[Doerr, Martin]

Hi,

I think it would be better to use C++ 2011 Strongly-typed Enums. Can we still not use them?
AFAIK all compilers which are used for jdk/jdk support that.

Best regards,
Martin


> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> David Holmes
> Sent: Dienstag, 27. August 2019 10:02
> To: Stefan Karlsson <stefan.karlsson at oracle.com>; hotspot-dev <hotspot-
> dev at openjdk.java.net>
> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
> constants
> 
> 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