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

[Vladimir Kozlov]

On 11/6/18 12:09 PM, Roman Kennke wrote:
> 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/

Good.

> 
> 
> 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?

Bot offset is common case for array access - we have to take it into account.
Accesses to header should not be Bot (except LoadKlass as comment said - for which there are checks). Length and 
markword should have fixed offsets.

Thanks,
Vladimir

> 
> 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
>>>
>