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

[Stuart Monteith]

Thanks for your comments Nick.

I've updated the code with your comments:
   http://cr.openjdk.java.net/~smonteith/oop64/webrev-20190211/



BR,
   Stuart

On Fri, 1 Feb 2019 at 02:50, Nick Gasson (Arm Technology China)
<Nick.Gasson at arm.com> wrote:
>
> Hi Stuart
>
> Just a few minor comments from looking through your patch.
>
> src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp
>
> +// SRDM: Use64BitLiteralAddresses isn't initialized for the static
> _patch_info_offset to be uses.
> +int PatchingStub::patch_info_offset() { return
> -NativeGeneralJump::get_instruction_size(); }
>
> "to be uses" -> "to be used"?
>
> src/hotspot/cpu/aarch64/globals_aarch64.hpp
>
> +  experimental(bool, Use64BitLiteralAddresses, false,                   \
> +          "Use 64 bit in literal addresses instead of 48 bits.")
>
> "Use 64 *bits* in ..." or "Use 64 bit literal addresses instead of 48 bit"?
>
> src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp
>
> +        // is therefore disables Use64BitLiteralAddresses until support
> is enabled.
>
> s/is/it/
>
> +    // mov, movk, movk, [movk] <- SRDM This fails with
> Use64BitLiteralAddresses
>
> Can we make this assert(!Use64BitLiteralAddresses, "...")?
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
>
> +
> +    if (Use64BitLiteralAddresses) {
> +      assert(nativeInstruction_at(insn_addr+12)->is_movk(), "wrong
> insns in patch");
> +      Instruction_aarch64::patch(insn_addr+12, 20, 5, (dest >>= 16) &
> 0xffff);
> +      instructions = 4;
> +    }
>
> Does it make sense to add `else assert(dest == 0, ">48 bit literal
> address without Use64BitLiteralAddresses");' here?
>
> src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp
>
> +     if (!Use64BitLiteralAddresses) {
> +       // movz, movk, movk.
> +       return 3 * 4;
> +     } else {
> +       return 4 * 4;
> +     }
>
> It's clearer if the * 4 is * NativeInstruction::instruction_size;
>
> src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp
>
> +      return 4 * 4;
>
> Same here.
>
> src/hotspot/share/aot/aotLoader.cpp
>
> +  if (Use64BitLiteralAddresses && UseAOT) {
> +    warning("64-bit Literal Addresses disabled due to UseAOT.");
> +    FLAG_SET_CMDLINE(bool, UseAOT, false);
> +  }
>
> Shouldn't it be FLAG_SET_CMDLINE(bool, Use64BitLiteralAddresses, false)
> to match the warning? Probably don't need to capitalize "Literal Addresses".
>
> Thanks,
> Nick
>
>
> On 31/01/2019 21:55, Stuart Monteith wrote:
> >     I've redone the patch such that whether or not it is an oop,
> > addresses are emitted and patched as 64-bit (movz + 3 x movk) when
> > -XX:+Use64BitLiteralAdddresses is passed.
> >
> > I've annotated some more sections to clarify what instructions are
> > being patched or sized, as well as some areas where it hasn't changed
> > - it was worthwhile to do thi
> > The Aarch64_specific_constant::instruction_size enum has been changed
> > to a method. This has encroached on shared code. Arguably this could
> > do with even more refactoring in NativeInst_aarch.(cpp|hpp) and the
> > users thereof if it is to be consistent.
> >
> > Regarding JVM CI and Graal - ZGC doesn't support them yet. Graal will
> > need to be changed to recognise the option and conditionally emit
> > 64-bit addresses (at least in
> > AArch64MacroAssembler::movNativeAddress(Register, long)
> > I've added some more logic to disable Use64BitLiteralAddresses when
> > UseAOT or JVMCI are enabled.
> >
> > The patch is:
> > http://cr.openjdk.java.net/~smonteith/oop64/webrev-20190130/
> >
> > I've opened a bug in JBS: https://bugs.openjdk.java.net/browse/JDK-8216491
> >
> > I've been trying to track down the source of some of the constants for
> > the sizes of code.  In one instance we have:
> > http://cr.openjdk.java.net/~smonteith/oop64/webrev-20190130/src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.hpp.udiff.html
> >
> > ... there are apparently 12 instructions for each call stub. The
> > emit_static_call_stub() method only uses 8 instructions, but with
> > Use64BitLiteralAddresses enabled the warning "compilation bailout:
> > exception handler overflow" is generated.
> > I've changed the code to emit 14 instructions instead, but I've yet to
> > find the call stub that requires 12 instructions.
> >
> > I'll follow up with the ZGC patch soon - there was a change upstream
> > to lea that needs to be accommodated.
> >
> > BR,
> >     Stuart
> >
> >
> >
> >
> >
> > On Mon, 3 Dec 2018 at 18:40, Andrew Haley <aph at redhat.com> wrote:
> >>
> >> On 12/3/18 4:53 PM, Stuart Monteith wrote:
> >>
> >>> 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.
> >>
> >> Could well be. I am starting to wonder if it is worth the effort.
> >> Maybe the best thing to do is have a global 48- to 64-bit switch
> >> and use it everywhere we want a pointer literal.
> >>
> >> Please don't put code of any complexity into header files. Put it
> >> into .c files instead.
> >>
> >> The logic in MacroAssembler::movoop is a already a mess, and
> >> adding an isOop flag to pass to doesn't help. I think it would
> >> make more sense to have explicit 48- and 64-bit mov methods, and
> >> let MacroAssembler::mov(Register, Address) control which one is
> >> called.
> >>
> >> This hunk makes no sense. The comment directly contradicts
> >> the code:
> >>
> >>    enum Aarch64_specific_constants {
> >>      instruction_size            =    3 * 4, // movz, movk, movk, [movk].  See movptr().
> >>      instruction_offset          =    0,
> >>      displacement_offset         =    0,
> >>    };
> >>
> >> It makes no sense to have instruction_size be a constant here.
> >> I guess it never did.
> >>
> >> --
> >> Andrew Haley
> >> Java Platform Lead Engineer
> >> Red Hat UK Ltd. <https://www.redhat.com>
> >> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671