[aarch64-port-dev ] RFD: AOT for AArch64

[Andrew Dinn]

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
-     * always generate the same output.
-     *
+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!


2)
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.


3)
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.*;


4)
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()) {
-                    masm.shl(64, resultRegister, resultRegister,
encoding.getShift());
+                    masm.lshr(64, resultRegister, resultRegister,
encoding.getShift());
                 }
             } else {

CompressPointer was doing an shl before? Really?


5)
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;


6)
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;


7)
compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64RestoreRegistersOp.java

redundant import:

import jdk.vm.ci.aarch64.AArch64Kind;


8)
compiler/src/org.graalvm.compiler.lir.aarch64/src/org/graalvm/compiler/lir/aarch64/AArch64SaveRegistersOp.java

redundant import:

import org.graalvm.collections.EconomicSet;