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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Bob Vandette
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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
I’ve run specJVM98 on the three 32-bit ARM VMs.

I also verified that Linux x64 builds.

Bob.


Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
CCing to build-dev and aarch64-port.

I looked through changes and they seem fine.

Thanks,
Vladimir

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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
> I’ve run specJVM98 on the three 32-bit ARM VMs.
>
> I also verified that Linux x64 builds.
>
> Bob.
>
>
Reply | Threaded
Open this post in threaded view
|

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

Aleksey Shipilev-4
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

Reply | Threaded
Open this post in threaded view
|

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

Bob Vandette
Thanks for the detailed review Aleksey. I took care of these.

Bob.

> On Sep 17, 2018, at 1:49 PM, Aleksey Shipilev <[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|

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

Mikael Vidstedt-3
In reply to this post by Bob Vandette

Looks good. Thanks for doing this!

Cheers,
Mikael

> On Sep 17, 2018, at 9:20 AM, Bob Vandette <[hidden email]> 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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
> I’ve run specJVM98 on the three 32-bit ARM VMs.
>
> I also verified that Linux x64 builds.
>
> Bob.
>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Bob Vandette
Hi Bob,

On 18/09/2018 2: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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
> I’ve run specJVM98 on the three 32-bit ARM VMs.

Did you test all the ARM related abi-profiles? It seems to me they were
only for Oracle's ARM32 port and may have never been used otherwise. I
would have expected all that stuff to be removed.

Cheers,
David

> I also verified that Linux x64 builds.
>
> Bob.
>
>
Reply | Threaded
Open this post in threaded view
|

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

Bob Vandette
I only did some basic testing of the hard-float abi.  Bell SW has offered to do more extensive testing
as part of this JEP.

I have no way of knowing if any of the other profiles are being used but I would think it’s
worth keeping them in the event that someone wants to try to build/test these other configurations.

The goal of this JEP is to eliminate the arm64 port and not cause any other changes in behavior.

Bob.


> On Sep 17, 2018, at 10:53 PM, David Holmes <[hidden email]> wrote:
>
> Hi Bob,
>
> On 18/09/2018 2: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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
>> I’ve run specJVM98 on the three 32-bit ARM VMs.
>
> Did you test all the ARM related abi-profiles? It seems to me they were only for Oracle's ARM32 port and may have never been used otherwise. I would have expected all that stuff to be removed.
>
> Cheers,
> David
>
>> I also verified that Linux x64 builds.
>> Bob.

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Bob,

On 18/09/2018 11:17 PM, Bob Vandette wrote:
> I only did some basic testing of the hard-float abi.  Bell SW has offered to do more extensive testing
> as part of this JEP.
>
> I have no way of knowing if any of the other profiles are being used but I would think it’s
> worth keeping them in the event that someone wants to try to build/test these other configurations.
>
> The goal of this JEP is to eliminate the arm64 port and not cause any other changes in behavior.

Sorry I was under the mistaken impression that all of the Oracle ARM
port was being removed, but it is only the 64-bit part.

That said it would concern me greatly if people are still building for
anything older than ARMv7 with MP support! The work I'm doing to always
act as-if MP is not only potentially inefficient on a uniprocessor, but
for ARM variants without MP support, potentially it won't even run if
instructions don't exist. I need to look into this further.

Thanks,
David

> Bob.
>
>
>> On Sep 17, 2018, at 10:53 PM, David Holmes <[hidden email]> wrote:
>>
>> Hi Bob,
>>
>> On 18/09/2018 2: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’ve testing building the remaining 32 and 64 bit ARM ports including the minimal, client and server VMs.
>>> I’ve run specJVM98 on the three 32-bit ARM VMs.
>>
>> Did you test all the ARM related abi-profiles? It seems to me they were only for Oracle's ARM32 port and may have never been used otherwise. I would have expected all that stuff to be removed.
>>
>> Cheers,
>> David
>>
>>> I also verified that Linux x64 builds.
>>> Bob.
>
Reply | Threaded
Open this post in threaded view
|

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

Boris Ulasevich
In reply to this post by Aleksey Shipilev-4
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
>
Reply | Threaded
Open this post in threaded view
|

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

Simon Nash
In reply to this post by David Holmes
On 19/09/2018 07:44, David Holmes wrote:

> Hi Bob,
>
> On 18/09/2018 11:17 PM, Bob Vandette wrote:
>> I only did some basic testing of the hard-float abi.  Bell SW has
>> offered to do more extensive testing
>> as part of this JEP.
>>
>> I have no way of knowing if any of the other profiles are being used
>> but I would think it’s
>> worth keeping them in the event that someone wants to try to
>> build/test these other configurations.
>>
>> The goal of this JEP is to eliminate the arm64 port and not cause any
>> other changes in behavior.
>
> Sorry I was under the mistaken impression that all of the Oracle ARM
> port was being removed, but it is only the 64-bit part.
>
> That said it would concern me greatly if people are still building for
> anything older than ARMv7 with MP support! The work I'm doing to always
> act as-if MP is not only potentially inefficient on a uniprocessor, but
> for ARM variants without MP support, potentially it won't even run if
> instructions don't exist. I need to look into this further.
>
> Thanks,
> David
>
David,
My build for arm-sflt needs to run on ARMv5 uniprocessor maschines and my build for arm-vfp-hflt needs to run on ARMv7
uniprocessor machines. Is the work you are doing that could cause problems with this included in JDK11 or just JDK12?

Simon

>> Bob.
>>
>>
>>> On Sep 17, 2018, at 10:53 PM, David Holmes <[hidden email]>
>>> wrote:
>>>
>>> Hi Bob,
>>>
>>> On 18/09/2018 2: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’ve testing building the remaining 32 and 64 bit ARM ports
>>>> including the minimal, client and server VMs.
>>>> I’ve run specJVM98 on the three 32-bit ARM VMs.
>>>
>>> Did you test all the ARM related abi-profiles? It seems to me they
>>> were only for Oracle's ARM32 port and may have never been used
>>> otherwise. I would have expected all that stuff to be removed.
>>>
>>> Cheers,
>>> David
>>>
>>>> I also verified that Linux x64 builds.
>>>> Bob.
>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Simon,

On 20/09/2018 7:48 AM, Simon Nash wrote:

> On 19/09/2018 07:44, David Holmes wrote:
>> Hi Bob,
>>
>> On 18/09/2018 11:17 PM, Bob Vandette wrote:
>>> I only did some basic testing of the hard-float abi.  Bell SW has
>>> offered to do more extensive testing
>>> as part of this JEP.
>>>
>>> I have no way of knowing if any of the other profiles are being used
>>> but I would think it’s
>>> worth keeping them in the event that someone wants to try to
>>> build/test these other configurations.
>>>
>>> The goal of this JEP is to eliminate the arm64 port and not cause any
>>> other changes in behavior.
>>
>> Sorry I was under the mistaken impression that all of the Oracle ARM
>> port was being removed, but it is only the 64-bit part.
>>
>> That said it would concern me greatly if people are still building for
>> anything older than ARMv7 with MP support! The work I'm doing to
>> always act as-if MP is not only potentially inefficient on a
>> uniprocessor, but for ARM variants without MP support, potentially it
>> won't even run if instructions don't exist. I need to look into this
>> further.
>>
>> Thanks,
>> David
>>
> David,
> My build for arm-sflt needs to run on ARMv5 uniprocessor maschines and
> my build for arm-vfp-hflt needs to run on ARMv7 uniprocessor machines.
> Is the work you are doing that could cause problems with this included
> in JDK11 or just JDK12?

This is for JDK 12. Of course the intent is to not cause problems for
anyone. The changes aim at simplifying the code whilst marginally
improving performance in the common case of a multi-processor system, at
the expense of potentially decreasing performance for uniprocessors.
Though as has been pointed out in my review thread, the existing use of
AssumeMP (default true) will be causing the same performance changes and
for spinlocks my changes will improve things for uniprocessors.

My area of concern is where instructions issued for the MP case may not
be valid on specific architectures. For example pldw is only available
on ARMv7 with multi-processor extensions. I need to be sure, for
example, only supported DMB/DSB variants are issued on ARMv5.

Thanks,
David

> Simon
>
>>> Bob.
>>>
>>>
>>>> On Sep 17, 2018, at 10:53 PM, David Holmes <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi Bob,
>>>>
>>>> On 18/09/2018 2: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’ve testing building the remaining 32 and 64 bit ARM ports
>>>>> including the minimal, client and server VMs.
>>>>> I’ve run specJVM98 on the three 32-bit ARM VMs.
>>>>
>>>> Did you test all the ARM related abi-profiles? It seems to me they
>>>> were only for Oracle's ARM32 port and may have never been used
>>>> otherwise. I would have expected all that stuff to be removed.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> I also verified that Linux x64 builds.
>>>>> Bob.
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Boris Ulasevich
In reply to this post by 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
>>
Reply | Threaded
Open this post in threaded view
|

Re: 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
>>>