Thanks for the detailed review Aleksey. I took care of these. Bob.
> On Sep 17, 2018, at 1:49 PM, Aleksey Shipilev <sh...@redhat.com> wrote: > > On 09/17/2018 07:00 PM, Vladimir Kozlov wrote: >> On 9/17/18 9:20 AM, Bob Vandette wrote: >>> Please review the changes related to JEP 340 which remove the second >>> 64-bit ARM port from the JDK.>> >>> http://cr.openjdk.java.net/~bobv/8209093/webrev.01 > > I read through the webrev, and it seems to be the clean removal. It also feels > very satisfying to drop 15 KLOC of ifdef-ed code. > > Minor nits: > > *) In src/hotspot/cpu/arm/c1_LIRAssembler_arm.cpp and > src/hotspot/cpu/arm/interp_masm_arm.cpp we have "#if defined(ASSERT)", which > cab > be just "#ifdef ASSERT" now > > *) Ditto in src/hotspot/cpu/arm/jniFastGetField_arm.cpp we have "#if > defined(__ABI_HARD__)" > > *) In src/hotspot/cpu/arm/macroAssembler_arm.hpp, the comment below seems to > apply to AArch64 only. Yet, only the first line of the comment is removed. I > think we should instead convert that comment into "TODO:" mentioning this > might > be revised after AArch64 removal? > > 992 void branch_if_negative_32(Register r, Label& L) { > 993 // Note about branch_if_negative_32() / branch_if_any_negative_32() > implementation for AArch64: > 994 // tbnz is not used instead of tst & b.mi because destination may be > out of tbnz range (+-32KB) > 995 // since these methods are used in LIR_Assembler::emit_arraycopy() to > jump to stub entry. > 996 tst_32(r, r); > 997 b(L, mi); > 998 } > > *) In src/hotspot/cpu/arm/stubGenerator_arm.cpp, lines L1204..1205, > L1435..1436 > can be merged together: > > 1203 // Generate the inner loop for shifted forward array copy (unaligned > copy). > 1204 // It can be used when bytes_per_count < wordSize, i.e. > 1205 // byte/short copy > > 1434 // Generate the inner loop for shifted backward array copy (unaligned > copy). > 1435 // It can be used when bytes_per_count < wordSize, i.e. > 1436 // byte/short copy > > > *) In src/hotspot/cpu/arm/stubGenerator_arm.cpp, the comment changed > incorrectly around L3363? > > - // R4 (AArch64) / SP[0] (32-bit ARM) - element count (32-bit int) > + // R4 - element count (32-bit int) > > > *) In src/hotspot/cpu/arm/templateInterpreterGenerator_arm.cpp, "R4"/"R5" > comments are missing: > > - const Register Rsig_handler = AARCH64_ONLY(R21) NOT_AARCH64(Rtmp_save0 > /* > R4 */); > - const Register Rnative_code = AARCH64_ONLY(R22) NOT_AARCH64(Rtmp_save1 > /* > R5 */); > + const Register Rsig_handler = Rtmp_save0; > + const Register Rnative_code = Rtmp_save1; > > Thanks, > -Aleksey >
