RFR: 8230203: Replace markWord enums with uintptr_t constants

[Doerr, Martin]

Hi Stefan,

> > I think it would be better to use C++ 2011 Strongly-typed Enums.
> 
> Could you elaborate why it would be better to use them in this case?

IMHO enums are better to read and edit. I don't like "static const uintptr_t" replicated 100 times.
In addition, static consts usually spam the binaries with stuff which is never needed while enums are only for the compiler.

I don't understand what keeps us from targeting JEP 347 to jdk14.

Best regards,
Martin


> -----Original Message-----
> From: Stefan Karlsson <stefan.karlsson at oracle.com>
> Sent: Dienstag, 27. August 2019 13:08
> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-dev <hotspot-
> dev at openjdk.java.net>; Kim Barrett <kim.barrett at oracle.com>
> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
> constants
> 
> Hi Martin,
> 
> On 2019-08-27 10:22, Doerr, Martin wrote:
> > Hi,
> >
> > I think it would be better to use C++ 2011 Strongly-typed Enums.
> 
> Could you elaborate why it would be better to use them in this case?
> 
> AFAICT these enums where only enums because that used to be the way
> constants were written in HotSpot. There are some comments around this in:
> https://bugs.openjdk.java.net/browse/JDK-8153116
> 
>   Can we still not use them?
> > AFAIK all compilers which are used for jdk/jdk support that.
> 
> I don't think we can go there until we get:
> https://openjdk.java.net/jeps/347 - JEP 347: Adopt C++14 Language
> Features in HotSpot
> 
> Thanks,
> StefanK
> 
> >
> > 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