RFC: How to deal with/abstract some offset-related Shenandoah changes?

[Roman Kennke]

Hi Vladimir,

thanks for the heads-up. Sure, I'll wait and try to come up with something reasonable myself in the meantime.

Thanks! /Roman

Am 16. Oktober 2018 02:05:11 MESZ schrieb Vladimir Kozlov <vladimir.kozlov at oracle.com>:
>Hi Roman,
>
>I and others looked on this and agree with you that
>BrooksPointer::byte_offset() checks and other 
>code you showed should be cleaned up. It may take time to come up with
>good suggestion. Please, wait.
>
>Regards,
>Vladimir
>
>On 10/12/18 1:57 AM, Roman Kennke wrote:
>> Hello,
>> 
>> I'd like to get some opinions on how should we deal with some changes
>> that we need in Shenandoah, that really look a bit ugly, but that
>seem a
>> little out of place in a GC interface too. Let me show you the code:
>> 
>>
>https://builds.shipilev.net/patch-openjdk-shenandoah-jdk/latest/src/hotspot/share/asm/assembler.cpp.udiff.html
>> 
>> There are 3 blocks there:
>> - The first deals with the aarch64 addressing bits. Only the lower
>> 48bits are used for addressing, and thus the upper 16 are masked
>away.
>> - the second block adjusts the offset by 8 bytes for Shenandoah when
>> computing the normalized heap-based-offset for narrow-oops. This is
>> needed to get combined decode+read-barrier instruction to work iirc.
>> - the last block allows implicit null-checks on Shenandoah fwd ptr
>loads
>> (now including the masking from block#1 above).
>> 
>> I really am not sure how do deal with this. It does look like it
>would
>> require a two-way abstraction:
>> 1. CPU specific. i.e. move this method under src/cpu/$CPU and do the
>> right thing there (possibly duplicating for platforms that do common
>stuff)
>> 2. GC specific to deal with the -8 offset of Shenandoah
>> 
>> However, I doubt that it would make the code any more easier to
>> understand/read/etc. And how would the GC abstraction look like? bool
>> BarrierSetAssembler::needs_explicit_null_check(..) ? And if GC says
>> 'false' then don't check any of the other stuff?
>> 
>> I'm a bit at a loss here. Any suggestions are appreciated.
>> 
>> 
>> There is another place that deals with special offsets:
>>
>https://builds.shipilev.net/patch-openjdk-shenandoah-jdk/latest/src/hotspot/share/ci/ciInstanceKlass.cpp.udiff.html
>> 
>> (look at get_canonical_holder() there)
>> 
>> Please let me know what you think.
>> 
>> Roman
>>