AArch64 Zero/SignExtend elision is inadequate

[adinn]

This patch fixes the problem reported here: #238

When running on AArch64 it adds a phase to the LowTier just before the Scheduling phase. This new phase replaces pairs of Read + Sign/ZeroExtend nodes with an AArch64ReadNode that combines the read and extend operation into one.

The patch includes a unit test (ZeroSignExtendTest) which exercises the node replacement for the newly added phase. Generated code for the test snippets before and after the patch shows that the patched version elides redundant stxw or andw instructions that the previous match rules could not eliminate.

Atached assembler dumps for the before and after code show the improvement.
extend-before.txt
extend-after.txt

[adinn]

n.b. bumped JDK9 version in Travis YAML so build can proceed

[dougxc]

Our internal gate checks that there are no Eclipse warnings. Is it possible for you to check this as well? We can't do it in Travis until openjdk8 binaries are available.

Finished Compiling org.graalvm.compiler.truffle.hotspot.test with ecj-daemon
----------
1. ERROR in /scratch/graaluser/buildslave/buildlog/ci_executor/main/compiler/src/org.graalvm.compiler.lir.jtt/src/org/graalvm/compiler/lir/jtt/StackStoreLoadTest.java (at line 82)
	private static final LIRTestSpecification stackCopy = new StackStoreLoadSpec();
	                                          ^^^^^^^^^
The value of the field StackStoreLoadTest.stackCopy is not used
----------
1 problem (1 error)
Result = -1

[adinn]

Hmm, I'm not sure what is happening here but I think that comments relates to a different PR.

[adinn]

Ah, ok, I think I understand what has happened. That file was in my last PR but is not yet in my tree.

The fix for PR #237 must now be in upstream (it wasn't when I created the PR). I will need to pull frm upstream and rebase this PR. Just a second ...

[adinn]

Hmm, no I don't seem to have StackStoreLoadTest in my tree -- did you mean to post this to the other PR?

[adinn]

I'm seeing an error from Travis here that I don't really understand. When I run mx canonicalizeprojects on my tree I see no errors. Any ideas?

[adinn]

Ah ok, my mistake: I now get this error

  File "/home/adinn/openjdk/graal/graal/compiler/mx.compiler/suite.py", line 330 in definition of org.graalvm.compiler.hotspot.aarch64:
Canonical dependencies for project org.graalvm.compiler.hotspot.aarch64 are: [
        "org.graalvm.compiler.hotspot",
        "org.graalvm.compiler.replacements.aarch64",
      ],
  File "/home/adinn/openjdk/graal/graal/compiler/mx.compiler/suite.py", line 693 in definition of org.graalvm.compiler.replacements.aarch64:
Canonical dependencies for project org.graalvm.compiler.replacements.aarch64 are: [
        "org.graalvm.compiler.core.aarch64",
        "org.graalvm.compiler.replacements",
      ],

Can anyone tell me what it means? n.b. my fix changed suite.py to allow my LowTier Node class AArch64ReadNode to see classes in org.graalvm.compiler.core.aarch64.

--- a/compiler/mx.compiler/suite.py
+++ b/compiler/mx.compiler/suite.py
@@ -696,6 +696,7 @@ suite = {
       "dependencies" : [
         "org.graalvm.compiler.replacements",
         "org.graalvm.compiler.lir.aarch64",
+        "org.graalvm.compiler.core.aarch64",
       ],
       "checkstyle" : "org.graalvm.compiler.graph",
       "javaCompliance" : "1.8",

No doubt that is the source of this problem. Is there some other way round this.

[tkrodriguez]

There's a canonical form for the contents of the dependencies so that we don't duplicate dependencies. When you change one you might then need to adjust others. So just make dependencies section of the projects it's complaining about look like it says.

[adinn]

Ok, I fixed the dependencies and the Travis build has now passed

[tkrodriguez]

I'll put this through. As an aside we've talked several times about doing exactly this but in a more general way. The FixReadsPhase actually provides a good time to do this now. We just need to have two stamps in a ReadNode, one for the size of the memory operation and one for the resulting type. There's already an API split for this stamp vs getAccessStamp we just need to propagate the split through the backends.

[adinn]

Thanks for accepting this as is.

I agree that a generic solution to this would be better in the long run. It should be straightforward to remove the AArch64-specific phase & read node when working a generic fix through to the back ends.

[gilles-duboscq]

Note that there is Node#hasExactlyOneUsage to cover this idiom without having to actually do the counting.