[aarch64-port-dev ] Review, please

[Johnson, E Andrew]

Thanks, Ed.  I will check out each individual patch and verify that (a) it is necessary, and (b) can't possibly be solved another way in the specific architecture files.  Thanks for the advice.

It is a curiosity why the override for inlineSmallCode exists for other architectures.  It looks like these were introduced as a "quick fix" that somehow persisted.

-AndyJ
________________________________________
From: Edward Nevill <edward.nevill at linaro.org>
Sent: Tuesday, April 14, 2015 5:21 AM
To: Johnson, E Andrew
Cc: aarch64-port-dev at openjdk.java.net
Subject: Re: [aarch64-port-dev ] Review, please

On Mon, 2015-04-13 at 15:13 +0000, Johnson, E Andrew wrote:

>
> See the attached file for the proposed patches.

Hi Andy,

Thanks for this patch.

I don't think a patch like this will be accepted upstream as is. Each case where there is a #if defined(AARCH64) needs to be separated out and a justification given for why it needs to be conditionalised on AARCH64.

Expect pushback on each case. These are changes to shared code and introduce conditionalised changes on a specific architecture which the upstream maintainers/reviewers really hate.

Start with the specific case you found which causes the performance regression.

  // Some inlining tuning
#if defined(X86) || defined(AARCH64)
  if (FLAG_IS_DEFAULT(InlineSmallCode)) {
    FLAG_SET_DEFAULT(InlineSmallCode, 2000);
  }
#endif

#ifdef SPARC
  if (FLAG_IS_DEFAULT(InlineSmallCode)) {
    FLAG_SET_DEFAULT(InlineSmallCode, 2500);
  }
#endif

The first question I would have as a reviewer (which I am not) is why are the defaults being modified here and why dont we simply set the correct defaults in globals_XXX.hpp

InlineSmallCode is declared product_pd in globals.hpp and has the following definitions in globals_XXX.hpp

hotspot/src/cpu/aarch64/vm/globals_aarch64.hpp:define_pd_global(intx, InlineSmallCode,          1000);
hotspot/src/cpu/ppc/vm/globals_ppc.hpp:define_pd_global(intx, InlineSmallCode,       1500);
hotspot/src/cpu/x86/vm/globals_x86.hpp:define_pd_global(intx, InlineSmallCode,          1000);
hotspot/src/cpu/zero/vm/globals_zero.hpp:define_pd_global(intx,  InlineSmallCode,      1000 );
hotspot/src/cpu/sparc/vm/globals_sparc.hpp:define_pd_global(intx, InlineSmallCode,       1500);


WRT most of the other #if defined(AARCH64) changes they were introduced to jdk8 in changeset 4754

changeset:   4754:961994affa01
user:        Edward Nevill ed at camswl.com
date:        Mon Jul 22 12:15:52 2013 +0100
files:       make/linux/platform_aarch64 src/os/linux/vm/os_linux.cpp src/share/vm/adlc/output_c.cpp src/share/vm/c1/c1_LIR.cpp src/share/vm/c1/c1_LIR.hpp src/share/vm/c1/c1_LIRAssembler.cpp src/share/vm/c1/c1_LIRGenerator.cpp src/share/vm/c1/c1_LinearScan.cpp src/share/vm/interpreter/interpreterRuntime.cpp src/share/vm/interpreter/interpreterRuntime.hpp src/share/vm/memory/allocation.inline.hpp src/share/vm/opto/machnode.hpp src/share/vm/prims/unsafe.cpp src/share/vm/runtime/advancedThresholdPolicy.cpp src/share/vm/runtime/vframeArray.cpp
description:
Remove -DAMD64 from sysdefs in platform_aarch64

So, initially for the builtin sim we had -DAMD64 in platform_aarch64. In fact I think it read -DAARCH64 -DAMD64. This was obviously broken, but it made sense at the time. In this changeset I added defined(AARCH64) to all #ifdef X86 statements that I thought were relevant at the time.

If these changes are to be pushed upstream a testcase will need for be found for each one which demonstrates that the conditionalisation is still necessary.

The other case in the changes you list was

--- a/src/share/vm/utilities/elfFile.cpp        Thu Apr 09 17:38:28 2015 -0700
+++ b/src/share/vm/utilities/elfFile.cpp        Fri Apr 10 15:06:38 2015 -0400
@@ -261,7 +261,12 @@
       }
     }
   }
+// x86 defaults to execstack, AARCH64 defaults to noexecstack
+#ifdef AARCH64
+  return true;
+#else
   return false;
+#endif
 }
 #endif

This was introduced in changeset 6668

changeset:   6668:0303ccd7b68d
user:        Edward Nevill edward.nevill at linaro.org
date:        Fri Feb 28 14:25:21 2014 +0000
files:       src/share/vm/utilities/elfFile.cpp
description:
Fix runtime/7107135/Test7107135 - problems with execstack

So, here the change was introduced to fix a specific test failure. Is the test still failing on jdk9? Does the above change still fix the test?

All the best,
Ed.