RFR: 8209093 - JEP 340: One AArch64 Port, Not Two

[Boris Ulasevich]

Hi Bob,

   I checked your changes with jtreg and jck. I see no new fails 
introduced by this change.

regards,
Boris

On 19.09.2018 13:30, Boris Ulasevich wrote:
> Hi Bob,
> 
> Thank you for this job!
> I have started testing. Will come back with results next week :)
> The changes is Ok for me. Please see some minor comments below.
> 
> regards,
> Boris
> 
> On 17.09.2018 20:49, Aleksey Shipilev 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. 
> 
> Yes, the note looks like AArch64 specific comment, but I think it is 
> valid for arm32 too. So the change is Ok for me.
> 
>> 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?
> 
> I believe both (L3188 and L3363) comments should mention SP[0] (not R4) 
> as an input parameter now.
> 
>>    - //    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
>>