RFR: JDK-8213371: GC/C2 abstraction and cleanup to handle custom offset for GC memory accesses

[Roman Kennke]

Hi Vladimir,

Thanks for reviewing!

> Changes in compile.cpp and type.cpp look fine.
> 
> I think in memnode.cpp (uint) casts are used to take into account
> OffsetTop and OffsetBot without explicitly checking for them. We don't
> expect OffsetTop here - there assert to check that. But 'off' could be
> OffsetBot. In line 1706 there is OffsetBot check but not in line 1737:
> 
> http://hg.openjdk.java.net/jdk/jdk/file/0e8084c8cbb7/src/hotspot/share/opto/memnode.cpp#l1737

Ah! I was wondering. So by casting we implicitly accept OffsetBot as
off_beyond_header=true. Ok.

> I think you need to add explicit check OffsetBot for off_beyond_header.
> You did that in type.cpp.

Yes. However, I think it's clearer to include "|| off ==
Type::OffsetBot" in the second use of off_beyond_header, rather than
including it first, and then excluding it in the first use. Like this:

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8213371/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8213371/webrev.01/

?

I must admit that I don't really understand the usefulness of including
OffsetBot in the 2nd clause, because that includes the case where offset
is not actually beyond the mark word, iow 'we don't know what the offset
is'. Right?

Thanks,
Roman


> Thanks,
> Vladimir
> 
> On 11/5/18 7:34 AM, Roman Kennke wrote:
>>   There are a few places in C2 where we need to deal with Shenandoah's
>> custom offset to load the forwarding pointer.
>>
>> This patch:
>> - Adds hook and verification to allow GC to flatten/initialize an alias
>> type for (e.g.) fwd ptr accesses (btw: the 'to' field in compile.cpp
>> around the changed code is unused)
>> - in memnode.cpp removes the (unnecessary?) cast from ints to uints
>> - in type.cpp, make the offset check more explicit about offset > 0 or
>> the two predefined offsets that could be negative.
>>
>> The latter two are related to the change insofar that they conflict with
>> Shenandoah's -8 offset, which would require extra handling without those
>> cleanups. Let me know if you want those changes separate.
>>
>> Testing: hotspot/jtreg:tier1
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8213371
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/JDK-8213371/webrev.00/
>>
>> Can I please get a review?
>>
>> Roman
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181106/3bac706a/signature.asc>