RFD: AOT for AArch64 (original) (raw)

Andrew Dinn adinn at redhat.com
Tue Mar 27 15:08:00 UTC 2018

On 27/03/18 09:18, Andrew Dinn wrote:

On 26/03/18 16:02, Andrew Dinn wrote:

On 26/03/18 15:56, Andrew Dinn wrote:

Ship it! Ok, so I know this really needs a code audit too. I'm working on that now. I am still trying to understand all the details of this patch so this is really just preliminary feedback. Many of the comments below are questions, posed mostly as requests for clarification rather than suggestions for any improvement. Also, I'm now switching to look at the Graal changes so I can tie these two patches together. Most of the answers to the last round involved removing the stuff that was asked about. So, I am now quite happy with the remaining hotspot changes (I'm still not clear why x86 has to zero its callInfopoint entries but clearly AArch64 /doesn't/ need -- we know it works -- to so that can pass).

Below is initial feedback on about the Graal changes. I didn't find anything much that needed changing nor come up with nay real questions about the code -- it's mostly unneeded imports. However, I think I still need to spend some more time piecing this together with the hotspot changes. I'll try to get final comments plus a yea or nay (well, ok I guess it's going to be a yea) posted by late tomorrow morning.

regards,

Andrew Dinn

Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

  1. compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/GraalError.java

    /** * This constructor creates a {@link GraalError} with a message

assembled via * {@link String#format(String, Object...)}. It always uses the ENGLISH locale in order to

+internal is * * @param msg the message that will be associated with the error, in String.format syntax * @param args parameters to String.format - parameters that implement {@link Iterable} will be * expanded into a [x, x, ...] representation.

This looks like an accidental paste-over!

  1. compiler/src/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotLIRGenerator.java

Two import issues:

+import static jdk.vm.ci.amd64.AMD64.rbp;

This import is not needed

import org.graalvm.compiler.core.common.calc.Condition; +import org.graalvm.compiler.core.common.spi.ForeignCallDescriptor; +import org.graalvm.compiler.core.common.spi.ForeignCallDescriptor;

ForeignCallDescriptor is imported twice.

  1. compiler/src/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotLoadAddressOp.java

redundant include

import static org.graalvm.compiler.asm.aarch64.AArch64Address.*;

  1. compiler/src/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotMove.java

@@ -117,7 +149,7 @@ public class AArch64HotSpotMove { } else if (nonNull) { masm.sub(64, resultRegister, ptr, base); if (encoding.hasShift()) {

encoding.getShift());

encoding.getShift()); } } else {

CompressPointer was doing an shl before? Really?

  1. compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64Call.java

redundant import:

import static jdk.vm.ci.aarch64.AArch64.lr;

  1. compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64Move.java

redundant import:

import static jdk.vm.ci.meta.JavaKind.Int;

  1. compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64RestoreRegistersOp.java

redundant import:

import jdk.vm.ci.aarch64.AArch64Kind;

  1. compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64SaveRegistersOp.java

redundant import:

import org.graalvm.collections.EconomicSet;

More information about the hotspot-dev mailing list