RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

[Erik Österlund]

Hi Kim,

Thank you for the review.

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00_01/

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8193063/webrev.01/

On 2018-01-08 20:23, Kim Barrett wrote:
>> On Dec 5, 2017, at 9:49 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi,
>>
>> In order to replace oopDesc::load_heap_oop() but with stricter memory ordering properties, like MO_VOLATILE, some Access tweaks are required to allow RawAccess<>::oop_load() to return narrowOop values.
>>
>> I made the necessary changes to allow narrowOop values for all RawAccess operations that have an address (not the _at variants).
>>
>> While I am at it, I thought I'd clean up a few things that bug me:
>>
>> * The decorator verification for memory ordering specifically did not work as I originally intended, leading to harder to decipher compiler errors deeper down when using the wrong memory ordering decorators
> I kind of wish that had been dealt with separately.  Oh well.
>
>> * An unnecessary include of oop.inline.hpp was removed from the G1 barrier set.
>>
>> This change helps solving the following bug:
>> https://bugs.openjdk.java.net/browse/JDK-8129440
>>
>> This bug is filed under:
>> https://bugs.openjdk.java.net/browse/JDK-8193063
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/
>>
>> Thanks,
>> /Erik
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>   525       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;
>   582       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;
>
> I don't see any use of these Raw types. They look like copy-paste as
> part of splitting into two specializations.

Fixed.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>
> Too bad we don't have C++17. I think making can_hardwire_raw constexpr
> and using the new "if constexpr" syntax would have been sufficient.

Indeed. I stopped for a second, cried a bit about the lack of constexpr, 
and then did it the C++03 way.

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>
> I was initially thinking I wanted names for these two expressions,
> since they are repeated a bunch of times:
>
> HasDecorator<decorators, AS_RAW>::value && CanHardwireRaw<decorators>::value
> HasDecorator<decorators, AS_RAW>::value && !CanHardwireRaw<decorators>::value
>
> But I think what I really want is the better function template SFINAE
> syntax allowed by C++11, e.g.
>
>    template<DecoratorSet decorators, typename T,
>             ENABLE_IF(HasDecorator<Decorators, AS_RAW>),
>             DISABLE_IF(CanHardwireRaw<decorators>)>
>    inline static T store(...) ...

Yes, we are a bit stuck with old school SFINAE for some time.

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>   796   // Step 2: Reduce types.
>
> This comment looks like it needs updating for the narrowOop value
> support.

Unfortunately, the comment was already accurate (and hence was 
inaccurate before).

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/accessBackend.hpp
>   176   AccessInternal::MustConvertCompressedOop<idecorators, T>::value,
>
> Wrong indentation.
>
> ------------------------------------------------------------------------------

Fixed.

Thanks,
/Erik