From patchwork Wed Oct 16 11:17:45 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Edward Nevill X-Patchwork-Id: 21072 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f70.google.com (mail-pa0-f70.google.com [209.85.220.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id D577E25B8B for ; Wed, 16 Oct 2013 11:17:51 +0000 (UTC) Received: by mail-pa0-f70.google.com with SMTP id fa1sf1180893pad.9 for ; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:subject:from:reply-to:to :cc:date:organization:mime-version:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :list-post:list-help:list-archive:list-unsubscribe:content-type :content-transfer-encoding; bh=C5InBdsut+KPPpkpNmdyUq+N60pRN8Nn5aJy6wahBho=; b=aR2kWXAtH20VGLtSoEByecKbHo+2ku36AB34y4Ua+cZqgHJVYfytPb2Q5U0o4rYX01 lv79/lGSuoghR9r2Re4xpjsaevXtUeirPIs0OMrPY56c/GPUmxrUJ/WbsILc0q12Pqs4 7UkUcYX6Cx/Pg8EVfwa1v3wiFA/E70iM3ZqUvrYI2AL4IWO6/xEC3P1eJjU0OUJqWNrv z6pw4K30voUeviOTHnpciserRlmdc/86WFyfw75nGcPhIFoaYaVouoGF4tQzKir+Zhaw yg6KfVegdzHLBTSd2N2mwsjDF24R4WnyALLrEZXR/9oA3rmrXtkPZjG0SY4PQqLCX9Wd Dshg== X-Received: by 10.67.14.70 with SMTP id fe6mr974218pad.15.1381922270463; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.0.116 with SMTP id 20ls539534qed.34.gmail; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) X-Received: by 10.52.243.138 with SMTP id wy10mr1622788vdc.2.1381922270292; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) Received: from mail-vc0-f179.google.com (mail-vc0-f179.google.com [209.85.220.179]) by mx.google.com with ESMTPS id dt10si24509766vdb.99.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 16 Oct 2013 04:17:50 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.220.179 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.220.179; Received: by mail-vc0-f179.google.com with SMTP id ht10so264810vcb.24 for ; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) X-Gm-Message-State: ALoCoQnyxE8F+54RAbYyGefffqGtSQLOjVFQyrgfEzVuNHwAqGW3qkfagrqviDJUcNNju4d6QuZ9 X-Received: by 10.58.210.234 with SMTP id mx10mr1921865vec.9.1381922270156; Wed, 16 Oct 2013 04:17:50 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.220.174.196 with SMTP id u4csp30726vcz; Wed, 16 Oct 2013 04:17:49 -0700 (PDT) X-Received: by 10.180.206.178 with SMTP id lp18mr1852600wic.40.1381922268694; Wed, 16 Oct 2013 04:17:48 -0700 (PDT) Received: from mail-wg0-f45.google.com (mail-wg0-f45.google.com [74.125.82.45]) by mx.google.com with ESMTPS id j9si775585wiy.36.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 16 Oct 2013 04:17:48 -0700 (PDT) Received-SPF: neutral (google.com: 74.125.82.45 is neither permitted nor denied by best guess record for domain of edward.nevill@linaro.org) client-ip=74.125.82.45; Received: by mail-wg0-f45.google.com with SMTP id z12so540740wgg.24 for ; Wed, 16 Oct 2013 04:17:48 -0700 (PDT) X-Received: by 10.194.109.68 with SMTP id hq4mr1997115wjb.12.1381922268110; Wed, 16 Oct 2013 04:17:48 -0700 (PDT) Received: from [192.168.1.140] (validation.linaro.org. [88.98.47.97]) by mx.google.com with ESMTPSA id s4sm4925441wiy.1.2013.10.16.04.17.46 for (version=SSLv3 cipher=RC4-SHA bits=128/128); Wed, 16 Oct 2013 04:17:47 -0700 (PDT) Message-ID: <1381922265.8150.19.camel@localhost.localdomain> Subject: RFR: Fix for UseCompressedClassPointers From: Edward Nevill Reply-To: edward.nevill@linaro.org To: "aarch64-port-dev@openjdk.java.net" Cc: patches@linaro.org Date: Wed, 16 Oct 2013 12:17:45 +0100 Organization: Linaro X-Mailer: Evolution 3.8.3 (3.8.3-2.fc19) Mime-Version: 1.0 X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: edward.nevill@linaro.org X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.220.179 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , 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 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 ---