[aarch64-port-dev ] Review, please

[Edward Nevill]

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.