[aarch64-port-dev ] RFC: 64 bit literal oops

[Stuart Monteith]

Hello Derek,
  Thanks for looking at this.

- I ran with and without the option enabled and checked for
differences. There were no significant differences.
- I have to admit, I haven't tried this with JVMCI/Graal.
- I'll take a look at enhancing the checking. I was more worried about
different parts of the codebase reading literals rather than reading
the same section.
- I could open a JIRA item for this. I wasn't sure whether or not it
would be appropriate to merge it as part of general ZGC support, but
there's nothing stopping me from submitting it as a separate item and
still quoting it as before for ZGC.
- The SRDM comments were left in there deliberately, to promote
discussion at this stage. I will, of course, remove them in due
course.

- I could make it an experimental feature. My actual thinking was for
it to be flag that wasn't externally visible, and only enabled along
with +UseZGC. But as the code could work equally well with any other
garbage collector, I could make it externally visible and test it and
ensure the other collectors were unaffected. What would your opinion
be?

- There is the computation and the ShouldNotReachHere() because I
originally just had the computation. However, during my testing I
added ShouldNotReachHere() as I believed the codepath wouldn't trip.
Both in place, as I expected that someone might suggest that I missed
a place that ought to have been adapted. As it is, I'm not 100% sure
that ShouldNotReachHere() won't ever trip - I would like to know what
people think.

There is another possible option, which is to just avoid 48-bit
literals, and just use 64-bit unconditionally everywhere. It would
mean that we could be easily consistent and 52-bit VA would be easier
to implement on top of it. Again, I seek people's opinions.

BR,
   Stuart





On Fri, 30 Nov 2018 at 01:50, White, Derek <Derek.White at cavium.com> wrote:
>
> Hi Stuart,
>
> General comments:
> - I really like splitting out the oop64 patch separately.
> - I think it would be useful to get this checked in separately from ZGC, so some of the rest of my suggestions point in that direction.
> - Testing:
>    - You mentioned testing jtreg with and without the patch:
>        - Did you test with Use64BitLiteralOops on, off, or both?
>    - Is this code exercised more by JVMCI/Graal? Test with Graal as well?
>    - Can we force more checking in debug mode?
>      - This might be overkill, but a read_literal function that reads & decodes a bunch of movz/movks would allow simple assert that the constant was written as expected.
> - It would be good to add an enhancement request/jira for this.
> - Are the SRDM questions resolved? Remove comments?
>
> cpu/aarch64/globals_aarch64.hpp
>  - Consider making this an experimental flag instead of product.
>
> cpu/aarch64/macroAssembler_aarch64.cpp
>  - line 293: Why are we both doing the computation and a ShouldNotReachHere()?
>
> Thanks,
>
>  - Derek
>
>
>
> ________________________________
> From: aarch64-port-dev <aarch64-port-dev-bounces at openjdk.java.net> on behalf of Stuart Monteith <stuart.monteith at linaro.org>
> Sent: Tuesday, October 2, 2018 5:54 PM
> To: aarch64-port-dev
> Cc: zgc-dev at openjdk.java.net
> Subject: [aarch64-port-dev ] RFC: 64 bit literal oops
>
> External Email
>
> Hello,
>    I've been testing my patch to make literal oops 64-bit rather than
> 48-bit. This is a necessary prerequisite for ZGC to operate on aarch64
> using tagged pointers. I'm not submitting this in it's final form, but
> just for a view on whether this is the proper way to do this.
>
> The patch is here:
> http://cr.openjdk.java.net/~smonteith/oop64/webrev-20181002/
>
> I've tested it against jtreg and G1GC, and get clean results as
> compared to without the patch. No issues are apparent running
> SPECjbb-2015.
>
> There are essentially 7 changes:
>    1. There is a new options: -XX:+Use64BitLiteralOops
>    2. MacroAssembler::patch_oop(address,address) patches 64-bit
> addresses if the flag is enabled.
>    3.  MacroAssembler::target_addr_for_insn(address, unsigned) is
> modified to pull out a 64-bit target address, if there is an extra
> movk, and then calls ShouldNotReachHere().
>    4. MacroAssembler::mov(Register, Address) has been changed to call
> movptr with a flag set if the address is an Oop.
>    5.  MacroAssembler::movptr(Register, uintptr_t) has been modified
> to take a flag indicating if it is an oop that is being emitted. If
> Use64BitLiteralOops has been set, then an additional movk is emitted
> to make up the 64 bit literal.
>    6. NativeInstruction::is_oop_at(int) was added, to detect if an oop
> exists at the address.
>    7. NativeMovConstReg::next_instruction_address() has been changed
> to handle get the address after "movz, movk, movk" and with 64 bit
> oops "movz, movk, movk, movk."
>
> (6) has never been observed,, so I don't consider it tested - there
> aren't circumstances where it returns true, so arguably it it should
> be replaced by an appropriate assert.
> Likewise, (3) is also an unnecessary change - the ShouldNotReachHere()
> never traps during testing - it was left in for review purposes.
>
> It has been mentioned to me that we could just use 64-bit literals, as
> that would make things simpler - I look forward to people's opinions.
> It was also mentioned ages ago that in future the coloured bits could
> be made irrelevant for ZGC.
>
> There is the up and coming 52-bit VA scheme for Arm v8. If we are to
> use >48 bits of address space we will have to ask for it explicitly,
> in which case we'd need to use 64 bit oops. The class metadata could
> remain 48-bit in that case. There is an interesting presentation on
> 52-bit VA here: https://connect.linaro.org/resources/yvr18/yvr18-119/
>
> BR,
>     Stuart