RFR: Fix for UseCompressedClassPointers

Message ID 1381922265.8150.19.camel@localhost.localdomain
State New
Headers show

Commit Message

Edward Nevill Oct. 16, 2013, 11:17 a.m.
Hi,

The following patch fixes UseCompressedClassPointers for C1 and C2 and re-enables them by default for C2.

Basically the change is that there are now 2 narrow bases, narrow_klass_base and narrow_oop_base. narrow_oop_base is kept in rheapbase as before.

This means that in encode_klass and decode_klass we must add/sub narrow_klass_base and we do not have it conveniently in a register.

Unfortunately rscratch1 and rscratch2 are variously used by callers of encode_klass and decode_klass. So what I do is use rheapbase as a scratch register if necessary, and then call reinit_heapbase afterwards to restore it. This mirrors what is done on x86.

It is not possible to call add(...) to simply add/sub either the narrow_klass_base or the delta between the narrow_klass_base and the narrow_oop_base because of the following in MacroAssembler::wrap_add_sub_imm_insn(...)

...
       assert_different_registers(Rd, Rn);
       mov(Rd, (uint64_t)imm);
       (this->*insn2)(Rd, Rn, Rd, LSL, 0);
...

Because of the limited range of add/sub handled on aarch64 (+/- 1<<24) we cannot use add/sub unless we can guarantee that the src and dst registers are different which leads to us needing a scratch register so we are back where we started.

I have also made a change in aarch64.ad to fix a case where encode_heap_oop_not_null was in fact being called with a null constant. Also I remove some bogus guarantees from nativeInst_aarch64 to make it work for C2.

I know the above solution is not ideal and we would prefer a more optimal solution, however it is at least correct so I would like to push it and look at ways of optimising later.

Possible optimisation is to add an additional arg to encode/decode klass which is the scratch register to be used. Then the callers can either pass in rscratch1, rscratch2, or rheapbase if they genuinely do not have a free scratch register.

Ok to push?
Ed.

--- CUT HERE ---
exporting patch:
# HG changeset patch
# User Edward Nevill edward.nevill@linaro.org
# Date 1381920878 -3600
#      Wed Oct 16 11:54:38 2013 +0100
# Node ID 778fdde3772e321c55fefa3df6f22c3e7d49cd33
# Parent  a99f56e36ea4ad581c901c44a27906919485e30c
Fix UseCompressedClassPointers

Comments

Andrew Haley Oct. 16, 2013, 3:08 p.m. | #1
On 10/16/2013 12:17 PM, Edward Nevill wrote:
> Ok to push?

OK, thanks.

Andrew.

Patch

diff -r a99f56e36ea4 -r 778fdde3772e src/cpu/aarch64/vm/aarch64.ad
--- a/src/cpu/aarch64/vm/aarch64.ad	Mon Oct 14 09:52:17 2013 +0100
+++ b/src/cpu/aarch64/vm/aarch64.ad	Wed Oct 16 11:54:38 2013 +0100
@@ -2375,7 +2375,7 @@ 
     // need to do this the hard way until we can manage relocs
     // for 32 bit constants
     __ movoop(rscratch2, (jobject)con);
-    __ encode_heap_oop_not_null(rscratch2);
+    if (con) __ encode_heap_oop_not_null(rscratch2);
     if (index == -1) {
       __ strw(rscratch2, Address(base, disp));
     } else {
@@ -2621,7 +2621,7 @@ 
       // need to do this the hard way until we can manage relocs
       // for 32 bit constants
       __ movoop(rscratch2, (jobject)con);
-      __ encode_heap_oop_not_null(rscratch2);
+      if (con) __ encode_heap_oop_not_null(rscratch2);
     }
     MOV_VOLATILE(rscratch2, $mem$$base, $mem$$index, $mem$$scale, $mem$$disp,
 		 rscratch1, stlrw);
@@ -2634,8 +2634,9 @@ 
       // need to do this the hard way until we can manage relocs
       // for 32 bit constants
       __ movoop(rscratch2, (jobject)con);
-      __ encode_heap_oop_not_null(rscratch2);
-      __ encode_klass_not_null(rscratch2);
+      // Either it is a heap oop or a klass pointer? It can't be both?
+      // __ encode_heap_oop_not_null(rscratch2);
+      if (con) __ encode_klass_not_null(rscratch2);
     }
     MOV_VOLATILE(rscratch2, $mem$$base, $mem$$index, $mem$$scale, $mem$$disp,
 		 rscratch1, stlrw);
diff -r a99f56e36ea4 -r 778fdde3772e src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
--- a/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp	Mon Oct 14 09:52:17 2013 +0100
+++ b/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp	Wed Oct 16 11:54:38 2013 +0100
@@ -1644,8 +1644,8 @@ 
 void MacroAssembler::reinit_heapbase()
 {
   if (UseCompressedOops) {
-    lea(rscratch1, ExternalAddress((address)Universe::narrow_ptrs_base_addr()));
-    ldr(rheapbase, Address(rscratch1));
+    lea(rheapbase, ExternalAddress((address)Universe::narrow_ptrs_base_addr()));
+    ldr(rheapbase, Address(rheapbase));
   }
 }
 
@@ -2078,73 +2078,46 @@ 
   }
 }
 
-void MacroAssembler::encode_klass_not_null(Register r) {
-#ifdef ASSERT
-  verify_heapbase("MacroAssembler::encode_klass_not_null: heap base corrupted?");
-#endif
-  if (Universe::narrow_klass_base() != NULL) {
-    sub(r, r, rheapbase);
-  }
-  if (Universe::narrow_klass_shift() != 0) {
-    assert (LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong");
-    lsr(r, r, LogKlassAlignmentInBytes);
-  }
-}
-
-void MacroAssembler::encode_klass_not_null(Register dst, Register src) {
-#ifdef ASSERT
-  verify_heapbase("MacroAssembler::encode_klass_not_null2: heap base corrupted?");
-#endif
-  if (dst != src) {
-    mov(dst, src);
-  }
-  if (Universe::narrow_klass_base() != NULL) {
-    sub(dst, dst, rheapbase);
-  }
-  if (Universe::narrow_klass_shift() != 0) {
-    assert (LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong");
-    lsr(dst, dst, LogKlassAlignmentInBytes);
-  }
-}
-
-void  MacroAssembler::decode_klass_not_null(Register r) {
-  // Note: it will change flags
-  assert (UseCompressedClassPointers, "should only be used for compressed headers");
-  // Cannot assert, unverified entry point counts instructions (see .ad file)
-  // vtableStubs also counts instructions in pd_code_size_limit.
-  // Also do not verify_oop as this is called by verify_oop.
-  if (Universe::narrow_klass_shift() != 0) {
-    assert(LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong");
-    if (Universe::narrow_klass_base() != NULL) {
-      add(r, rheapbase, r, Assembler::LSL, LogKlassAlignmentInBytes);
-    } else {
-      add(r, zr, r, Assembler::LSL, LogKlassAlignmentInBytes);
-    }
-  } else {
-    assert (Universe::narrow_klass_base() == NULL, "sanity");
-  }
-}
-
-void  MacroAssembler::decode_klass_not_null(Register dst, Register src) {
-  // Note: it will change flags
-  assert (UseCompressedClassPointers, "should only be used for compressed headers");
-  // Cannot assert, unverified entry point counts instructions (see .ad file)
-  // vtableStubs also counts instructions in pd_code_size_limit.
-  // Also do not verify_oop as this is called by verify_oop.
-  if (Universe::narrow_klass_shift() != 0) {
-    assert(LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong");
-    if (Universe::narrow_klass_base() != NULL) {
-      add(dst, rheapbase, src, Assembler::LSL, LogKlassAlignmentInBytes);
-    } else {
-      add(dst, zr, src, Assembler::LSL, LogKlassAlignmentInBytes);
-    }
-  } else {
-    assert (Universe::narrow_klass_base() == NULL, "sanity");
-    if (dst != src) {
-      mov(dst, src);
-    }
-  }
-}
+void MacroAssembler::encode_klass_not_null(Register dst, Register src) { 
+  Register rbase = dst; 
+#ifdef ASSERT 
+  verify_heapbase("MacroAssembler::encode_klass_not_null2: heap base corrupted?"); 
+#endif 
+  if (dst == src) rbase = rheapbase; 
+  mov(rbase, (uint64_t)Universe::narrow_klass_base()); 
+  sub(dst, src, rbase); 
+  if (Universe::narrow_klass_shift() != 0) { 
+    assert (LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong"); 
+    lsr(dst, dst, LogKlassAlignmentInBytes); 
+  } 
+  if (dst == src) reinit_heapbase(); 
+} 
+ 
+void MacroAssembler::encode_klass_not_null(Register r) { 
+  encode_klass_not_null(r, r); 
+} 
+ 
+void  MacroAssembler::decode_klass_not_null(Register dst, Register src) { 
+  Register rbase = dst; 
+  assert(Universe::narrow_klass_base() != NULL, "Base should be initialized"); 
+  assert (UseCompressedClassPointers, "should only be used for compressed headers"); 
+  // Cannot assert, unverified entry point counts instructions (see .ad file) 
+  // vtableStubs also counts instructions in pd_code_size_limit. 
+  // Also do not verify_oop as this is called by verify_oop. 
+  if (dst == src) rbase = rheapbase; 
+  mov(rbase, (uint64_t)Universe::narrow_klass_base()); 
+  if (Universe::narrow_klass_shift() != 0) { 
+    assert(LogKlassAlignmentInBytes == Universe::narrow_klass_shift(), "decode alg wrong"); 
+    add(dst, rbase, src, Assembler::LSL, LogKlassAlignmentInBytes); 
+  } else { 
+    add(dst, rbase, src); 
+  } 
+  if (dst == src) reinit_heapbase(); 
+} 
+ 
+void  MacroAssembler::decode_klass_not_null(Register r) { 
+  decode_klass_not_null(r, r); 
+} 
 
 // TODO
 //
diff -r a99f56e36ea4 -r 778fdde3772e src/cpu/aarch64/vm/nativeInst_aarch64.cpp
--- a/src/cpu/aarch64/vm/nativeInst_aarch64.cpp	Mon Oct 14 09:52:17 2013 +0100
+++ b/src/cpu/aarch64/vm/nativeInst_aarch64.cpp	Wed Oct 16 11:54:38 2013 +0100
@@ -143,19 +143,6 @@ 
 void NativeJump::insert(address code_pos, address entry) { Unimplemented(); }
 
 void NativeJump::check_verified_entry_alignment(address entry, address verified_entry) {
-  // Patching to not_entrant can happen while activations of the method are
-  // in use. The patching in that instance must happen only when certain
-  // alignment restrictions are true. These guarantees check those
-  // conditions.
-  const int linesize = 64;
-
-  // Must be wordSize aligned
-  guarantee(((uintptr_t) verified_entry & (wordSize -1)) == 0,
-            "illegal address for code patching 2");
-  // First 5 bytes must be within the same cache line - 4827828
-  guarantee((uintptr_t) verified_entry / linesize ==
-            ((uintptr_t) verified_entry + 4) / linesize,
-            "illegal address for code patching 3");
 }
 
 
diff -r a99f56e36ea4 -r 778fdde3772e src/share/vm/runtime/arguments.cpp
--- a/src/share/vm/runtime/arguments.cpp	Mon Oct 14 09:52:17 2013 +0100
+++ b/src/share/vm/runtime/arguments.cpp	Wed Oct 16 11:54:38 2013 +0100
@@ -1471,13 +1471,10 @@ 
     }
     FLAG_SET_DEFAULT(UseCompressedClassPointers, false);
   } else {
-// ECN: FIXME - UseCompressedClassPointers is temporarily broken
-#ifndef AARCH64
     // Turn on UseCompressedClassPointers too
     if (FLAG_IS_DEFAULT(UseCompressedClassPointers)) {
       FLAG_SET_ERGO(bool, UseCompressedClassPointers, true);
     }
-#endif
     // Check the CompressedClassSpaceSize to make sure we use compressed klass ptrs.
     if (UseCompressedClassPointers) {
       if (CompressedClassSpaceSize > KlassEncodingMetaspaceMax) {
--- CUT HERE ---