Re: RFR: 8199138: Add RISC-V support to Zero

[Thomas Stüfe]

Looks still good.

..Thomas

On Tue 7. Apr 2020 at 01:20, John Paul Adrian Glaubitz <
glaub...@physik.fu-berlin.de> wrote:

> Hello!
>
> On 4/6/20 8:09 PM, John Paul Adrian Glaubitz wrote:
> > I have reduced the complexity of the patch as some of the changes from
> > the previous change set are not necessary, in particular the changes
> > to config.{guess,sub}, the definition of EM_RISCV (which is already
> defined
> > by the Linux kernel headers now provided the kernel is recent enough).
>
> After checking various kernel headers of older but still supported
> enterprise
> Linux distributions such as SLE-12 and RHEL-7, I think it's probably better
> to include the redundant EM_RISCV definition to avoid build problems on
> these
> platforms [1]. I just want to be on the safe side.
>
> The build changes are unchanged.
>
> Adrian
>
> > [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.03/
>
> --
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaub...@debian.org
> `. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
>   `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
>

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

Hello!

On 4/6/20 8:09 PM, John Paul Adrian Glaubitz wrote:
> I have reduced the complexity of the patch as some of the changes from
> the previous change set are not necessary, in particular the changes
> to config.{guess,sub}, the definition of EM_RISCV (which is already defined
> by the Linux kernel headers now provided the kernel is recent enough).

After checking various kernel headers of older but still supported enterprise
Linux distributions such as SLE-12 and RHEL-7, I think it's probably better
to include the redundant EM_RISCV definition to avoid build problems on these
platforms [1]. I just want to be on the safe side.

The build changes are unchanged.

Adrian

> [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.03/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Thomas Stüfe]

Hi Adrian, looks good to me.

Cheers, Thomas

On Mon, Apr 6, 2020, 20:11 John Paul Adrian Glaubitz <
glaub...@physik.fu-berlin.de> wrote:

> Hello!
>
> Please review this small change which adds basic support for the riscv64
> target for Linux/Zero [1].
>
> I have reduced the complexity of the patch as some of the changes from
> the previous change set are not necessary, in particular the changes
> to config.{guess,sub}, the definition of EM_RISCV (which is already defined
> by the Linux kernel headers now provided the kernel is recent enough).
>
> Additionally, I'm leaving the change for os::get_summary_cpu_info() out
> as I would like to clean up this code a bit first - it's rather
> inconsistent
> considering whether the arch override is used for Hotspot arches only or
> also for Zero arches.
>
> Thanks,
> Adrian
>
> > [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.02/
>
> --
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaub...@debian.org
> `. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
>   `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
>

Re: RFR: 8199138: Add RISC-V support to Zero

[Erik Joelsson]

Build change looks good.

/Erik

On 2020-04-06 11:09, John Paul Adrian Glaubitz wrote:

Hello!

Please review this small change which adds basic support for the riscv64
target for Linux/Zero [1].

I have reduced the complexity of the patch as some of the changes from
the previous change set are not necessary, in particular the changes
to config.{guess,sub}, the definition of EM_RISCV (which is already defined
by the Linux kernel headers now provided the kernel is recent enough).

Additionally, I'm leaving the change for os::get_summary_cpu_info() out
as I would like to clean up this code a bit first - it's rather inconsistent
considering whether the arch override is used for Hotspot arches only or
also for Zero arches.

Thanks,
Adrian


[1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.02/

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

Hi Aleksey!

On 2/17/20 9:07 AM, Aleksey Shipilev wrote:
> On 2/12/20 6:13 PM, Aleksey Shipilev wrote:
>> On 2/12/20 6:00 PM, John Paul Adrian Glaubitz wrote:
>>> On 2/12/20 5:59 PM, Aleksey Shipilev wrote:
 On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote:
> I assume I can push with those changes and mark it as Reviewed-by: erikj, 
> shade?

 Mark it, yes. I believe non-trivial (yet exceedingly simple) things like 
 these require waiting for
 24 hours to anyone else to chime in with comments. There seem to be no 
 rush to get it in, right?
>>>
>>> No rush, no. Just wanted to make sure that it's fine to push then :).
>>
>> Yes, I believe it would be fine to push then.
> 
> I think it is fine to push now ;)

I have come up with a better and cleaner approach now which I will
post later this week. But I want to get the fix for JDK-8239001 out
first.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Aleksey Shipilev]

On 2/12/20 6:13 PM, Aleksey Shipilev wrote:
> On 2/12/20 6:00 PM, John Paul Adrian Glaubitz wrote:
>> On 2/12/20 5:59 PM, Aleksey Shipilev wrote:
>>> On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote:
 I assume I can push with those changes and mark it as Reviewed-by: erikj, 
 shade?
>>>
>>> Mark it, yes. I believe non-trivial (yet exceedingly simple) things like 
>>> these require waiting for
>>> 24 hours to anyone else to chime in with comments. There seem to be no rush 
>>> to get it in, right?
>>
>> No rush, no. Just wanted to make sure that it's fine to push then :).
> 
> Yes, I believe it would be fine to push then.

I think it is fine to push now ;)

-- 
Thanks,
-Aleksey

Re: RFR: 8199138: Add RISC-V support to Zero

[David Holmes]

This seems fine to me too.

Thanks,
David

On 13/02/2020 3:08 am, John Paul Adrian Glaubitz wrote:

Hi!

On 2/12/20 5:51 PM, Aleksey Shipilev wrote:

Neat. Looks good to me.

Minor nits in os_linux.cpp:

*) Can you move the comment to the #define line, as it is done in the similar 
blocks in the same file?

1854 #ifndef EM_RISCV  /* RISCV */
1855   #define EM_RISCV  243
1856 #endif

*) I believe this one is sorted alphabetically, so RISCV should be between 
__powerpc64__ and S390?

-AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__,
__powerpc64__, S390, SH, __sparc
+AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__,
__powerpc64__, S390, SH, __sparc, RISCV


I have done that now. Updated RFR in [1].

Adrian


[1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.01/

Re: RFR: 8199138: Add RISC-V support to Zero

[Aleksey Shipilev]

On 2/12/20 6:00 PM, John Paul Adrian Glaubitz wrote:
> On 2/12/20 5:59 PM, Aleksey Shipilev wrote:
>> On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote:
>>> I assume I can push with those changes and mark it as Reviewed-by: erikj, 
>>> shade?
>>
>> Mark it, yes. I believe non-trivial (yet exceedingly simple) things like 
>> these require waiting for
>> 24 hours to anyone else to chime in with comments. There seem to be no rush 
>> to get it in, right?
> 
> No rush, no. Just wanted to make sure that it's fine to push then :).

Yes, I believe it would be fine to push then.

-Aleksey

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

Hi!

On 2/12/20 5:51 PM, Aleksey Shipilev wrote:
> Neat. Looks good to me.
> 
> Minor nits in os_linux.cpp:
> 
> *) Can you move the comment to the #define line, as it is done in the similar 
> blocks in the same file?
> 
> 1854 #ifndef EM_RISCV  /* RISCV */
> 1855   #define EM_RISCV  243
> 1856 #endif
> 
> *) I believe this one is sorted alphabetically, so RISCV should be between 
> __powerpc64__ and S390?
> 
> -AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
> __powerpc__,
> __powerpc64__, S390, SH, __sparc
> +AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
> __powerpc__,
> __powerpc64__, S390, SH, __sparc, RISCV

I have done that now. Updated RFR in [1].

Adrian

> [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.01/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

On 2/12/20 5:59 PM, Aleksey Shipilev wrote:
> On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote:
>> I assume I can push with those changes and mark it as Reviewed-by: erikj, 
>> shade?
> 
> Mark it, yes. I believe non-trivial (yet exceedingly simple) things like 
> these require waiting for
> 24 hours to anyone else to chime in with comments. There seem to be no rush 
> to get it in, right?

No rush, no. Just wanted to make sure that it's fine to push then :).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Aleksey Shipilev]

On 2/12/20 5:54 PM, John Paul Adrian Glaubitz wrote:
> I assume I can push with those changes and mark it as Reviewed-by: erikj, 
> shade?

Mark it, yes. I believe non-trivial (yet exceedingly simple) things like these 
require waiting for
24 hours to anyone else to chime in with comments. There seem to be no rush to 
get it in, right?

-- 
Thanks,
-Aleksey

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

Hi!

On 2/12/20 5:51 PM, Aleksey Shipilev wrote:
> On 2/12/20 5:14 PM, John Paul Adrian Glaubitz wrote:
>>> [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.00/
> 
> Neat. Looks good to me.
> 
> Minor nits in os_linux.cpp:
> 
> *) Can you move the comment to the #define line, as it is done in the similar 
> blocks in the same file?
> 
> 1854 #ifndef EM_RISCV  /* RISCV */
> 1855   #define EM_RISCV  243
> 1856 #endif

Yes.

> *) I believe this one is sorted alphabetically, so RISCV should be between 
> __powerpc64__ and S390?
> 
> -AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
> __powerpc__,
> __powerpc64__, S390, SH, __sparc
> +AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
> __powerpc__,
> __powerpc64__, S390, SH, __sparc, RISCV

Sure.

I assume I can push with those changes and mark it as Reviewed-by: erikj, shade?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Aleksey Shipilev]

On 2/12/20 5:14 PM, John Paul Adrian Glaubitz wrote:
>> [1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.00/

Neat. Looks good to me.

Minor nits in os_linux.cpp:

*) Can you move the comment to the #define line, as it is done in the similar 
blocks in the same file?

1854 #ifndef EM_RISCV  /* RISCV */
1855   #define EM_RISCV  243
1856 #endif

*) I believe this one is sorted alphabetically, so RISCV should be between 
__powerpc64__ and S390?

-AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__,
__powerpc64__, S390, SH, __sparc
+AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__,
__powerpc64__, S390, SH, __sparc, RISCV

-- 
Thanks,
-Aleksey

Re: RFR: 8199138: Add RISC-V support to Zero

[Erik Joelsson]

Build changes look ok to me. Someone from hotspot should review the cpp 
file.


/Erik

On 2020-02-12 08:14, John Paul Adrian Glaubitz wrote:

Hi!

This is an updated RFR to add basic RISC-V support to Zero.

This patch is being used for the riscv64 port in Debian.

Please review the changes in [1].

Thanks,
Adrian


[1] http://cr.openjdk.java.net/~glaubitz/8199138/webrev.00/

Re: RFR: 8199138: Add RISC-V support to Zero

[Edward Nevill]

On Tue, 2018-03-27 at 17:46 +0900, John Paul Adrian Glaubitz wrote:
> On 03/27/2018 05:23 PM, Edward Nevill wrote:
> > Sorry for the delay. I was doing another test build on qemu which takes 
> > about 3 days.
> > 
> > 
> What confuses me: Why RISCV here and not RISCV64?
> 
> In particular this hunk:
> 
> @@ -1758,6 +1761,7 @@
>  {EM_PARISC,  EM_PARISC,  ELFCLASS32, ELFDATA2MSB, (char*)"PARISC"},
>  {EM_68K, EM_68K, ELFCLASS32, ELFDATA2MSB, (char*)"M68k"},
>  {EM_AARCH64, EM_AARCH64, ELFCLASS64, ELFDATA2LSB, (char*)"AARCH64"},
> +{EM_RISCV,   EM_RISCV,   ELFCLASS64, ELFDATA2LSB, (char*)"RISCV"},
>};
> 
> I know there is already 32-bit RISC-V and there are actually plans for
> using it. So, it looks to me you would be breaking 32-bit RISC-V here.
> 

We could do something like

{EM_RISCV,   EM_RISCV,   LP64_ONLY(ELFCLASS64) NOT_LP64(ELFCLASS32),
ELFDATA2LSB, (char*)"RISCV"},

Would this work?

All the best,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[Edward Nevill]

On Tue, 2018-03-27 at 17:46 +0900, John Paul Adrian Glaubitz wrote:
> On 03/27/2018 05:23 PM, Edward Nevill wrote:
> > @@ -1733,6 +1733,9 @@
> >  #ifndef EM_AARCH64
> >#define EM_AARCH64183   /* ARM AARCH64 */
> >  #endif
> > +#ifndef EM_RISCV  /* RISCV */
> > +  #define EM_RISCV  243
> > +#endif
> 
> What confuses me: Why RISCV here and not RISCV64?
> 
> In particular this hunk:
> 
> @@ -1758,6 +1761,7 @@
>  {EM_PARISC,  EM_PARISC,  ELFCLASS32, ELFDATA2MSB,
> (char*)"PARISC"},
>  {EM_68K, EM_68K, ELFCLASS32, ELFDATA2MSB,
> (char*)"M68k"},
>  {EM_AARCH64, EM_AARCH64, ELFCLASS64, ELFDATA2LSB,
> (char*)"AARCH64"},
> +{EM_RISCV,   EM_RISCV,   ELFCLASS64, ELFDATA2LSB,
> (char*)"RISCV"},
>};
> 
> I know there is already 32-bit RISC-V and there are actually plans
> for
> using it. So, it looks to me you would be breaking 32-bit RISC-V
> here.
> 

Because that is what is defined in elf.h

>From /usr/include/elf.h

#define EM_RISCV243 /* RISC-V */

There is no EM_RISCV32 or EM_RISCV64 in elf.h

All the best,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

On 03/27/2018 05:23 PM, Edward Nevill wrote:
> Sorry for the delay. I was doing another test build on qemu which takes about 
> 3 days.
> 
> Please review the following webrev
> 
> http://cr.openjdk.java.net/~enevill/8199138/webrev.02
> 
> This has the following additional changes over the previous webrev
> 
> 1) Add comment in os_linux.cpp
> 
> @@ -1733,6 +1733,9 @@
>  #ifndef EM_AARCH64
>#define EM_AARCH64183   /* ARM AARCH64 */
>  #endif
> +#ifndef EM_RISCV  /* RISCV */
> +  #define EM_RISCV  243
> +#endif

What confuses me: Why RISCV here and not RISCV64?

In particular this hunk:

@@ -1758,6 +1761,7 @@
 {EM_PARISC,  EM_PARISC,  ELFCLASS32, ELFDATA2MSB, (char*)"PARISC"},
 {EM_68K, EM_68K, ELFCLASS32, ELFDATA2MSB, (char*)"M68k"},
 {EM_AARCH64, EM_AARCH64, ELFCLASS64, ELFDATA2LSB, (char*)"AARCH64"},
+{EM_RISCV,   EM_RISCV,   ELFCLASS64, ELFDATA2LSB, (char*)"RISCV"},
   };

I know there is already 32-bit RISC-V and there are actually plans for
using it. So, it looks to me you would be breaking 32-bit RISC-V here.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Edward Nevill]

Hi,

On Tue, 2018-03-27 at 14:10 +0900, John Paul Adrian Glaubitz wrote:
> On 03/24/2018 02:26 AM, Magnus Ihse Bursie wrote:
> > 
> > On 2018-03-20 14:54, Edward Nevill wrote:
> > > Thanks for this. I have updated the webrev with the above comment.
> > > 
> > > http://cr.openjdk.java.net/~enevill/8199138/webrev.01
> > 
> > I note that in platform.m4 (sorry I didn't say this earlier), you set the 
> > CPU_ARCH to riscv64 as well, and not just riscv. Now I don't know how 
> > likely it is
> > that OpenJDK will ever support the 32-bit version of riscv, but it seems 
> > like it would make more sense to define the CPU_ARCH as "riscv", and the 
> > CPU as "riscv64".
> > 
> > It's just a minor thing, if you like it the way it is, keep it.
> 
> I agree, this is a bit odd.
> 
> @Edward: Is this correct as it currently is? Would be great if this changeset
> could finally get merged as Debian just recently bootstrapped riscv64 and
> is now building packages on real hardware with 10 build machines running:
> 

Sorry for the delay. I was doing another test build on qemu which takes about 3 
days.

Please review the following webrev

http://cr.openjdk.java.net/~enevill/8199138/webrev.02

This has the following additional changes over the previous webrev

1) Add comment in os_linux.cpp

@@ -1733,6 +1733,9 @@
 #ifndef EM_AARCH64
   #define EM_AARCH64183   /* ARM AARCH64 */
 #endif
+#ifndef EM_RISCV  /* RISCV */
+  #define EM_RISCV  243
+#endif

   static const arch_t arch_array[]={
 {EM_386, EM_386, ELFCLASS32, ELFDATA2LSB, (char*)"IA 32"},

2) Add RISCV to the #error list in os_linux.cpp

@@ -1794,7 +1800,7 @@
   static  Elf32_Half running_arch_code=EM_SH;
 #else
 #error Method os::dll_load requires that one of following is defined:\
-AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__, __powerpc64__, S390, SH, __sparc
+AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, PARISC, 
__powerpc__, __powerpc64__, S390, SH, __sparc, RISCV
 #endif

   // Identify compatability class for VM's architecture and library's 
architecture

3) Use 'riscv' instead of 'riscv64' for VAR_CPU_ARCH in platform.m4

@@ -114,6 +114,12 @@
   VAR_CPU_BITS=64
   VAR_CPU_ENDIAN=little
   ;;
+riscv64)
+  VAR_CPU=riscv64
+  VAR_CPU_ARCH=riscv
+  VAR_CPU_BITS=64
+  VAR_CPU_ENDIAN=little
+  ;;

4) Add riscv to the list of arch which do not have -m64 in flags.m4

@@ -237,7 +237,8 @@
 MACHINE_FLAG="-q${OPENJDK_TARGET_CPU_BITS}"
   elif test "x$TOOLCHAIN_TYPE" != xmicrosoft; then
 if test "x$OPENJDK_TARGET_CPU" != xaarch64 &&
-test "x$OPENJDK_TARGET_CPU" != xarm; then
+test "x$OPENJDK_TARGET_CPU" != xarm &&
+test "x$OPENJDK_TARGET_CPU" != xriscv64; then
   MACHINE_FLAG="-m${OPENJDK_TARGET_CPU_BITS}"
 fi
   fi

(This is necessary to get it building again. The previous webrev was based on a 
rev which did not have the -m64 problem)

I have run this through submit-hs with no problems and as mentioned have also 
done a complete rebuild under qemu for riscv.

Thanks for your patience,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

On 03/24/2018 02:26 AM, Magnus Ihse Bursie wrote:
> 
> On 2018-03-20 14:54, Edward Nevill wrote:
>> Thanks for this. I have updated the webrev with the above comment.
>>
>> http://cr.openjdk.java.net/~enevill/8199138/webrev.01
> I note that in platform.m4 (sorry I didn't say this earlier), you set the 
> CPU_ARCH to riscv64 as well, and not just riscv. Now I don't know how likely 
> it is
> that OpenJDK will ever support the 32-bit version of riscv, but it seems like 
> it would make more sense to define the CPU_ARCH as "riscv", and the CPU as 
> "riscv64".
> 
> It's just a minor thing, if you like it the way it is, keep it.

I agree, this is a bit odd.

@Edward: Is this correct as it currently is? Would be great if this changeset
could finally get merged as Debian just recently bootstrapped riscv64 and
is now building packages on real hardware with 10 build machines running:

> https://buildd.debian.org/status/architecture.php?a=riscv64=sid

I assume the build dependencies for OpenJDK in Debian will be built in around
a week or so. Until I then, we should have sorted this patch out so I can
add a (backported) patch to Debian's openjdk-8/9/10/11 packages.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Magnus Ihse Bursie]

On 2018-03-20 14:54, Edward Nevill wrote:

On Tue, 2018-03-20 at 08:39 +0100, Erik Helin wrote:

Please review the following webrev

Bugid: https://bugs.openjdk.java.net/browse/JDK-8199138
Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00

32 # First, filter out everything that doesn't begin with "aarch64-"
33 if ! echo $* | grep '^aarch64-\|^riscv64-' >/dev/null ; then

Could you please update the comment on line 32 to say the same thing as
the code?


Hi Eirk,

Thanks for this. I have updated the webrev with the above comment.

http://cr.openjdk.java.net/~enevill/8199138/webrev.01
I note that in platform.m4 (sorry I didn't say this earlier), you set 
the CPU_ARCH to riscv64 as well, and not just riscv. Now I don't know 
how likely it is that OpenJDK will ever support the 32-bit version of 
riscv, but it seems like it would make more sense to define the CPU_ARCH 
as "riscv", and the CPU as "riscv64".


It's just a minor thing, if you like it the way it is, keep it.

/Magnus



I have also fixed a problem encountered with the submit-hs repo where the build 
machine had older headers which did not define EM_RISCV.

The solution is to define EM_RISCV if not already defined as is done for 
aarch64.

IE.

  #ifndef EM_AARCH64
#define EM_AARCH64183   /* ARM AARCH64 */
  #endif
+#ifndef EM_RISCV
+  #define EM_RISCV  243
+#endif

This now passes the submit-hs tests.

Does this look OK to push now?

Thanks,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[Erik Helin]

On 03/20/2018 02:54 PM, Edward Nevill wrote:

On Tue, 2018-03-20 at 08:39 +0100, Erik Helin wrote:

Please review the following webrev


Bugid: https://bugs.openjdk.java.net/browse/JDK-8199138
Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00


32 # First, filter out everything that doesn't begin with "aarch64-"
33 if ! echo $* | grep '^aarch64-\|^riscv64-' >/dev/null ; then

Could you please update the comment on line 32 to say the same thing as
the code?



Hi Eirk,

Thanks for this. I have updated the webrev with the above comment.

http://cr.openjdk.java.net/~enevill/8199138/webrev.01


Please also update the error message at line 1802 - 1804:

1802 #error Method os::dll_load requires that one of following is 
defined:\
1803 AARCH64, ALPHA, ARM, AMD64, IA32, IA64, M68K, MIPS, MIPSEL, 
PARISC, __powerpc__, __powerpc64__, S390, SH, __sparc

1804 #endif


I have also fixed a problem encountered with the submit-hs repo where the build 
machine had older headers which did not define EM_RISCV.

The solution is to define EM_RISCV if not already defined as is done for 
aarch64.

IE.

  #ifndef EM_AARCH64
#define EM_AARCH64183   /* ARM AARCH64 */
  #endif
+#ifndef EM_RISCV
+  #define EM_RISCV  243
+#endif


Maybe add a corresponding /* RISC-V */ comment to use the same style as 
the other defines?



This now passes the submit-hs tests.


Ok, good.


Does this look OK to push now?


Please send out a final webrev first.

Thanks,
Erik


Thanks,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[John Paul Adrian Glaubitz]

On 03/19/2018 05:19 AM, Edward Nevill wrote:
> Interestingly, there is no implementation of atomic_copy64 for ARM32. I guess 
> it just relies on the compiler generating LDRD/STRD correctly and  doesn't 
> support earlier ARM32 archs. I'll do a bit of investigation.

I am planning to add arch-specific implementations for m68k and sh in the near
future. From the current build logs in Debian, it seems that the JVM is actually
hanging on these architectures from time to time and I think this could probably
be related to atomic_copy64 actually not being 100% atomic. I already added the
one for PowerPCSPE.

It's also interesting that there is no implementation for 32-Bit MIPS either.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: RFR: 8199138: Add RISC-V support to Zero

[Edward Nevill]

On Tue, 2018-03-20 at 08:39 +0100, Erik Helin wrote:
> Please review the following webrev
> > 
> > Bugid: https://bugs.openjdk.java.net/browse/JDK-8199138
> > Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00
> 
>32 # First, filter out everything that doesn't begin with "aarch64-"
>33 if ! echo $* | grep '^aarch64-\|^riscv64-' >/dev/null ; then
> 
> Could you please update the comment on line 32 to say the same thing as 
> the code?
> 

Hi Eirk,

Thanks for this. I have updated the webrev with the above comment.

http://cr.openjdk.java.net/~enevill/8199138/webrev.01

I have also fixed a problem encountered with the submit-hs repo where the build 
machine had older headers which did not define EM_RISCV.

The solution is to define EM_RISCV if not already defined as is done for 
aarch64.

IE.

 #ifndef EM_AARCH64
   #define EM_AARCH64183   /* ARM AARCH64 */
 #endif
+#ifndef EM_RISCV
+  #define EM_RISCV  243
+#endif

This now passes the submit-hs tests.

Does this look OK to push now?

Thanks,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[Erik Joelsson]

Build changes look ok to me.

/Erik


On 2018-03-17 12:02, Edward Nevill wrote:

Hi,

Please review the following webrev

Bugid: https://bugs.openjdk.java.net/browse/JDK-8199138
Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00

This webrev add Zero support for RISC-V

I propose to set up a project to develop template interpreter, C1 & C2
support for RISC-V and I will file a JEP for that work. This patch just
gets RISC-V building with Zero.

Many thanks,
Ed.

Re: RFR: 8199138: Add RISC-V support to Zero

[Andrew Haley]

On 03/18/2018 08:19 PM, Edward Nevill wrote:
> Pretty much. The only atomic operation which doesn't used GCC builtins is 
> os::atomic_copy64. For RISC-V this just does the same as all other 64 bit 
> CPUs.
> 
> *(jlong *) dst = *(const jlong *) src;

That's probably wrong, but it'll do for now.  We'll need something better
in the future.  GCC's __atomic_{load,store} (__ATOMIC_RELAXED) would do it.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8199138: Add RISC-V support to Zero

[Edward Nevill]

On Sun, 2018-03-18 at 14:37 +, Andrew Haley wrote:
> On 03/17/2018 07:02 PM, Edward Nevill wrote:
> > Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00
> > 
> > This webrev add Zero support for RISC-V
> 
> What happens with atomics?  Do we fall back to GCC builtins for everything?
> 

Pretty much. The only atomic operation which doesn't used GCC builtins is 
os::atomic_copy64. For RISC-V this just does the same as all other 64 bit CPUs.

*(jlong *) dst = *(const jlong *) src;

Interestingly, there is no implementation of atomic_copy64 for ARM32. I guess 
it just relies on the compiler generating LDRD/STRD correctly and  doesn't 
support earlier ARM32 archs. I'll do a bit of investigation.

For reference here is the implementation of atomic_copy64.

Regards,
Ed.

--- CUT ---
  static void atomic_copy64(const volatile void *src, volatile void *dst) {
#if defined(PPC32) && !defined(__SPE__)
double tmp;
asm volatile ("lfd  %0, %2\n"
  "stfd %0, %1\n"
  : "="(tmp), "=Q"(*(volatile double*)dst)
  : "Q"(*(volatile double*)src));
#elif defined(PPC32) && defined(__SPE__)
long tmp;
asm volatile ("evldd  %0, %2\n"
  "evstdd %0, %1\n"
  : "="(tmp), "=Q"(*(volatile long*)dst)
  : "Q"(*(volatile long*)src));
#elif defined(S390) && !defined(_LP64)
double tmp;
asm volatile ("ld  %0, 0(%1)\n"
  "std %0, 0(%2)\n"
  : "=r"(tmp)
  : "a"(src), "a"(dst));
#else
*(jlong *) dst = *(const jlong *) src;
#endif
--- CUT ---

Re: RFR: 8199138: Add RISC-V support to Zero

[Andrew Haley]

On 03/17/2018 07:02 PM, Edward Nevill wrote:
> Webrev: http://cr.openjdk.java.net/~enevill/8199138/webrev.00
> 
> This webrev add Zero support for RISC-V

What happens with atomics?  Do we fall back to GCC builtins for everything?

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671