[8u] RFR: cumulative patch required for C1 integration

[Edward Nevill]

On Wed, 2016-02-17 at 15:10 +0000, Sergey Nazarkin wrote:
> Hi!
> 
> Azul team continues its work on AArch32 port. We’ve achieved some results. 
> To facilitate future synchronization, we would like to share our work  that touches non-JIT part of the code base. 
> http://cr.openjdk.java.net/~snazarki/preC1/
> 
> 
> Sergey Nazarkin

Hi Sergey,

I have tested this on jdk8u and jdk9 and it looks fine.

I have a few comments on the patch, mostly cosmetic.

--- In src/cpu/aarch32/vm/assembler_aarch32.cpp

There seem to be a few more compiler warnings generated by this patch. We should aim to get the no. of compiler warnings down to 0. This is not so much of an issue with 8, but 9 by default builds with warnings treated as errors so I have to build with --disable-warnings-as-errors

Here are a couple. It would be helpful if you could fix any more you come across.

+bool Address::is_safe_for(InsnDataType type) {
+  switch (_acc_mode) {
+    case imm:
+    case lit:
+      return offset_ok_for_immed(_offset, type);
+    case reg:
+      return shift_ok_for_index(_shift, type);
+    case no_mode:
+    default:
+      ShouldNotReachHere();
+  }
 }

Needs a 'return false' at the end to prevent the warning "No return from value generating function"

--- In src/cpu/aarch32/vm/interpreter_aarch32.cpp

-void InterpreterGenerator::generate_transcendental_entry(AbstractInterpreter::MethodKind kind, int fpargs) {
-  address fn = NULL;
+void InterpreterGenerator::generate_transcendental_entry(AbstractInterpreter::MethodKind kind) {
+  address fn;


The 'fn = NULL' here was to prevent the warning 'Variable may be used uninitialised'.

--- In src/cpu/aarch32/vm/relocInfo_aarch32.cpp

2 occurrences of

+#if 0
+    warning("TODO: poll_Relocation::fix_relocation_after_move: "
+    "ensure relocating does nothing on relative instruction");
+#endif

I think this can be replaced with

  assert(false, "TODO: ...");

or delete it if this is not in fact the case.

--- In src/cpu/aarch32/vm/nativeInst_aarch32.hpp

-  enum { instruction_size = 4 };
+  enum { arm_insn_sz = 4 };

instruction_size seems to have changed throughout to arm_insn_sz

Is there a reason for this? Do you plan to add a 'thumb_insn_sz' at some point?

  // TODO remove this, every command is 4byte long
#if 1

I think we should just leave the methods in anyway. They are doing no harm and will be inlined in any case. I think the comment and the #if 1 should be deleted.

--- In src/cpu/aarch32/vm/macroAssembler_aarch32.cpp

 void MacroAssembler::far_call(Address entry, CodeBuffer *cbuf, Register tmp) {
   assert(CodeCache::find_blob(entry.target()) != NULL,
          "destination of far call not found in code cache");
+  // TODO performance issue: if intented to patch later,
+  // generate mov rX, imm; bl rX far call (to reserve space)
+#if 0
   if (far_branches()) {
+#else
+  if (entry.rspec().type() != relocInfo::none || far_branches()) {
+#endif


All the far_call and far_branch and trampoline code was to support code caches larger than 128M on aarch64. This was particularly necessary for C2 with tiered compilation where the code cache could grow quite large.

I think we should strip all of this back out. If we do need to support large code caches in the future then we should engineer it back in.

This will be a better approach than trying to carry the code in the aarch32 port where it will rust and break in any case.

However for the purpose of this patch I think it can be left as is. Just another item on the TODO list:-)

--- src/cpu/aarch32/vm/stubGenerator_aarch32.cpp

-      rbp_off = frame::arg_reg_save_area_bytes/BytesPerInt,
+      rfp_off = 0,

-    // lr and fp are already in place
-    assert(frame::arg_reg_save_area_bytes == 0, "please modify this code");
-    // __ sub(sp, rfp, frame::arg_reg_save_area_bytes + wordSize); // prolog
+    assert(is_even(framesize), "sp not 8-byte aligned");

This seems to back out my change

http://cr.openjdk.java.net/~enevill/8148355/webrev/hotspot.changeset

which was discussed on the list

http://mail.openjdk.java.net/pipermail/aarch32-port-dev/2016-January/000078.html

Was this intentional?



Otherwise this patch looks fine. Could you please prepare a new patch taking into account my comments above and I will push it.

All the best,
Ed.