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

[Aleksei Voitylov]

Bob,

Thank you for doing this. In the meanwhile, some of my fixes were pushed 
that invalidated your diff, for which I apologize. Here is an updated 
version of your patch which applies cleanly:

http://cr.openjdk.java.net/~avoitylov/webrev.8209093.02/

-Aleksei


On 23/09/2018 18:45, Boris Ulasevich wrote:
> 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
>>>