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

[Vladimir Kozlov]

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
>