RFR(L): 8161259: Simplify including platform files.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Jul 20 08:47:25 UTC 2016
Hi David,
that's great what you are saying and just the design I would expect!
> We did not want to have to
> pollute the shared sources with two sets of ifdefs for "64-bit ARM" so
> we worked with the Open port to ensure that shared code guarded by
> AARCH64 is suitable for both. We also ensured ARM was used to identify
> word-size agnostic code and we introduced ARM32 in a handful of places
> that needed it. And sometimes we have to be careful and ensure that
> ifdef chains check AARCH64 before they check ARM.
I think as a consequence the open AARCH port should define ARM, too.
I checked the occurrences and only see three places where this would have
an effect and would have to be fixed somehow:
*** share/vm/jvmci/vmStructs_jvmci.cpp:
<global>[610] #if defined(AARCH64) && !defined(ARM)
==> Would this break the closed port if defined?
(This is the only place where TARGET_ARCH_aarch64 was used)
*** share/vm/c1/c1_LIRGenerator.cpp:
load_item_force[253] #if !defined(ARM) && !defined(E500V2)
==> Would this break the open port if defined?
*** share/vm/c1/c1_Runtime1.cpp:
<global>[1154] #ifdef ARM
==> Would this break the open port if defined?
All the cases below are pointless.
> So what I'm suggesting is just not getting rid of those defines, but
> keeping them so they can be used as include guards (or other conditional
> code) when needed, and where the macros are not suitable.
I think it's quite misleading to have two defines that are 99% equivalent.
If we really need a special case here, you should define -DARM_CLOSED
or the like in your closed port. Such a name would make clear what's the
issue. At least, only your closed port has this problem.
Best regards,
Goetz.
These should not break the open port if ARM gets defined, or can be fixed easily.
---------------------------------------------------------
*** share/vm/c1/c1_LIR.cpp:
print[1519] #elif defined(ARM)
==> Use ARM32 as after AARCH64 in if-cascade.
*** os/linux/vm/os_linux.cpp:
dll_load[1796] #elif (defined ARM)
get_summary_cpu_info[2273] #elif defined(ARM)
==> Use ARM32 as after AARCH64 in if-cascade.
*** share/vm/opto/matcher.cpp:
init_first_stack_mask[558] #ifdef ARM
==> Should be ARM32 (Is under !LP64).
*** share/vm/c1/c1_LIR.cpp:
validate_type[212] ARM_ONLY(|| kindfield == cpu_register)
validate_type[219] ARM_ONLY(|| kindfield == cpu_register)
==> Just an assertion. Will just relax the check if defined in open AARCH64.
But maybe should be guarded by __SOFTFP__.
<global>[70] #if defined(ARM) || defined(AARCH64) || defined(PPC64)
==> Fine: ||
*** share/vm/c1/c1_LIR.hpp:
<global>[447] #if defined(SPARC) || defined(ARM) || defined(PPC) || defined(AARCH64)
<global>[537] #if defined(X86) || defined(ARM) || defined(AARCH64)
==> Fine: ||
*** share/vm/interpreter/interpreterRuntime.hpp:
defined[162] #if defined(IA32) || defined(AMD64) || defined(ARM)
*** share/vm/interpreter/interpreterRuntime.cpp:
<global>[1358] #if defined(IA32) || defined(AMD64) || defined(ARM)
==> Just defines a method which would be unused, should be fine.
These are in code not used in the open AARCH64 port:
--------------------------------------------------
*** os/bsd/vm/os_bsd.cpp:
<global>[215] #elif defined(ARM)
*** os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp:
<global>[102] #ifdef ARM
*** os_cpu/bsd_zero/vm/orderAccess_bsd_zero.inline.hpp:
<global>[31] #ifdef ARM
*** os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp:
<global>[102] #ifdef ARM
*** os_cpu/linux_zero/vm/orderAccess_linux_zero.inline.hpp:
<global>[31] #ifdef ARM
*** share/vm/utilities/macros.hpp:
<global>[434] #ifdef ARM
*** os/bsd/vm/os_bsd.cpp:
dll_load[1508] #elif (defined ARM)
dll_load[1524] IA32, AMD64, IA64, __sparc, __powerpc__, ARM, S390, ALPHA, MIPS, MIPSEL, PARISC, M68K
*** os/solaris/vm/os_solaris.cpp:
dll_load[1725] #elif (defined ARM)
dll_load[1729] IA32, AMD64, IA64, __sparc, __powerpc__, ARM, ARM
*** os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp:
store[164] #if !defined(ARM) && !defined(M68K)
store_ptr[171] #if !defined(ARM) && !defined(M68K)
add[178] #ifdef ARM
add_ptr[190] #ifdef ARM
xchg[230] #ifdef ARM
xchg_ptr[253] #ifdef ARM
cmpxchg[275] #ifdef ARM
cmpxchg_ptr[298] #ifdef ARM
*** os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp:
add[172] #ifdef ARM
add_ptr[184] #ifdef ARM
xchg[224] #ifdef ARM
xchg_ptr[247] #ifdef ARM
cmpxchg[269] #ifdef ARM
cmpxchg_ptr[292] #ifdef ARM
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 19. Juli 2016 23:59
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> <kim.barrett at oracle.com>
> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: RFR(L): 8161259: Simplify including platform files.
>
> Hi Goetz,
>
> On 19/07/2016 10:12 PM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> > we also have "closed ports", currently HPUX, ia64 and s390.
> > (Parisc is gone, puh!).
> > We basically patch all the shared changes onto the sources after
> > getting them via our licensee channel.
> > I think you should fix your closed port not to re-use the names of the
> > main openJdk platforms!
>
> Nobody "owns" a define of AARCH64 or ARM. We did not want to have to
> pollute the shared sources with two sets of ifdefs for "64-bit ARM" so
> we worked with the Open port to ensure that shared code guarded by
> AARCH64 is suitable for both. We also ensured ARM was used to identify
> word-size agnostic code and we introduced ARM32 in a handful of places
> that needed it. And sometimes we have to be careful and ensure that
> ifdef chains check AARCH64 before they check ARM.
>
> This has all been working quite nicely, as the include guards used, for
> example, TARGET_ARCH_AARCH64 and TARGET_ARCH_ARM - which are
> never
> defined at the same time (derived from HOTSPOT_TARGET_CPU_ARCH). But
> the
> current changes remove those unique defines and, before the HEADER_H
> forms were introduced, tried to use simple AARCH64 and ARM as include
> guards, and that doesn't work as they are not mutually exclusive.
>
> So what I'm suggesting is just not getting rid of those defines, but
> keeping them so they can be used as include guards (or other conditional
> code) when needed, and where the macros are not suitable.
>
> Thanks,
> David
> -----
>
> > I have no idea what hardware is addressed by your closed ports,
> > nor how you merge sources.
> > Is there also a port that sets
> > -DTARGET_ARCH_arm
> > -DARM
> > -DARM32
> > ?
> >
> > To fix this either you define
> > #if defined(ARM) && defined(_LP64)
> > #define ARM64
> > #endif
> > in macros.hpp or you set -DARM64 on the command line.
> > Then you replace all
> > #ifdef AARCH64
> > by
> > #if defined(AARCH64) || defined(ARM64)
> > or maybe it suffices altogether f you replace
> > #ifdef AARCH64
> > by
> > #if defined(AARCH64) || defined(ARM)
> >
> > For ppc, when we did the port we knew (and that's all we knew)
> > that you have a 32-bit port. Therefore we set up these macros
> > as on x86, where there is one for the arch (X86) and two for LP64/!LP64
> > (IA32, AMD64). This allowed to separate code for the closed port
> > (guarded by PPC32), the open port (PPC64) and shared for both (PPC).
> >
> > But I don't think it is necessary to do any further changes now, assuming
> > my change works for you as I adapted it. So we're all set I guess!
> >
> > Best regards,
> > Goetz.
> >
> >
> >
> >
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Tuesday, July 19, 2016 1:23 PM
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >> <kim.barrett at oracle.com>
> >> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >> dev at openjdk.java.net
> >> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>
> >> On 19/07/2016 7:08 PM, Lindenmaier, Goetz wrote:
> >>> Hi David,
> >>>
> >>>> I'm still uneasy that TARGET_ARCH_xxx is now just xxx. This kind of
> >>>> workaround is obscure - you have to know that the Open Aarch64 port
> >>>> defines AARCH64 but not ARM and so that code is for the Open port
> use
> >>>> only. There's no issue for the OS defines, but I wonder - just something
> >>>> to thing about - if TARGET_ARCH_xxx should be restored?
> >>>
> >>> Well, I think TARGET_ARCH_xxx always was xxx.
> >>> And I'm uneasy that it is no more. How do you handle that? You have to
> >>> check every AARCH change by RedHat against your closed port?
> >>
> >> The sources for the two ports are distinct so the only place we have to
> >> have a convention is in shared code that has CPU specific stuff and in
> >> the build files.
> >>
> >> The open Aarch64 port sets (among others):
> >> -DTARGET_ARCH_aarch64
> >> -DAARCH64
> >>
> >> the closed port sets
> >>
> >> -DTARGET_ARCH_arm
> >> -DARM
> >> -DAARCH64
> >>
> >> so it is the value of TARGET_ARCH_xxx that distinguishes them.
> Whenever
> >> you saw TARGET_ARCH_arm in open shared code it is/was referring to
> our
> >> closed port; and TARGET_ARCH_aarch64 refers to the open Aarch64 port.
> >>
> >> Of course TARGET_OS_ARCH_linux_xxx is in the same position.
> >>
> >> We need to keep a clear distinction. Using the combination of AARCH64
> >> and ARM is not so clear.
> >>
> >> You could easily have similar issues with other port groups - eg ppc64
> >> vs ppc32 vs ppcle.
> >>
> >> Thanks,
> >> David
> >>
> >>> I don't know about the closed stuff, but aarch came up recently, and
> >>> before that it sure was equivalent. And it still is the case for openJDK.
> >>>
> >>> Below output is grepped out of the make/<os>/platform_<cpu> files in
> >>> jdk8/hotspot, and none of the cpu/arch names are defined twice.
> >>> Zero is an exception I guess, as it's no real cpu/arch.
> >>>
> >>> Best regards,
> >>> Goetz.
> >>>
> >>> sysdefs = -DAIX -DPPC64
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DAMD64
> >>> sysdefs = -D_ALLBSD_SOURCE -DSPARC_WORKS -D_GNU_SOURCE -
> >> DAMD64
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DIA32
> >>> sysdefs = -D_ALLBSD_SOURCE -DSPARC_WORKS -D_GNU_SOURCE -
> DIA32
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DIA64 -DCC_INTERP
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DSPARC
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DSPARC
> >>> sysdefs = -D_ALLBSD_SOURCE -D_GNU_SOURCE -DCC_INTERP -DZERO -
> >> D at ZERO_ARCHDEF@ -DZERO_LIBARCH=\"@ZERO_LIBARCH@\"
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DAMD64
> >>> sysdefs = -DLINUX -DSPARC_WORKS -D_GNU_SOURCE -DAMD64
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DIA32
> >>> sysdefs = -DLINUX -DSPARC_WORKS -D_GNU_SOURCE -DIA32
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DIA64 -DCC_INTERP
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DPPC64
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DSPARC
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DSPARC
> >>> sysdefs = -DLINUX -D_GNU_SOURCE -DCC_INTERP -DZERO -
> >> D at ZERO_ARCHDEF@ -DZERO_LIBARCH=\"@ZERO_LIBARCH@\"
> >>> sysdefs = -DSOLARIS -DSPARC_WORKS -DAMD64
> >>> sysdefs = -DSOLARIS -D_GNU_SOURCE -DAMD64
> >>> sysdefs = -DSOLARIS -DSPARC_WORKS -DIA32
> >>> sysdefs = -DSOLARIS -D_GNU_SOURCE -DIA32
> >>> sysdefs = -DSOLARIS -DSPARC_WORKS -DSPARC
> >>> sysdefs = -DSOLARIS -D_GNU_SOURCE -DSPARC
> >>> sysdefs = -DSOLARIS -DSPARC_WORKS -DSPARC
> >>> sysdefs = -DSOLARIS -D_GNU_SOURCE -DSPARC
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>> Sent: Tuesday, July 19, 2016 10:47 AM
> >>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >>>> <kim.barrett at oracle.com>
> >>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>> dev at openjdk.java.net
> >>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>
> >>>> Thanks Goetz. Will get back to you asap if there are any further issues,
> >>>> but the incremental changes look okay.
> >>>>
> >>>> On 19/07/2016 5:47 PM, Lindenmaier, Goetz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> I added macros for C headers:
> >>>>> CPU_HEADER_H() etc.
> >>>>> and fixed the other issues mentioned by David and Coleen in this new
> >>>> webrev:
> >>>>> http://cr.openjdk.java.net/~goetz/wr16/8161259-
> >> newPfmIncl/webrev.03/
> >>>>>
> >>>>> I also added comments that AARCH64 and ARM are defined
> >>>>> at the same time.
> >>>>>
> >>>>> Further I edited vmStructs_jvmci.cpp to
> >>>>> #if defined(AARCH64) && !defined(ARM)
> >>>>
> >>>> I'm still uneasy that TARGET_ARCH_xxx is now just xxx. This kind of
> >>>> workaround is obscure - you have to know that the Open Aarch64 port
> >>>> defines AARCH64 but not ARM and so that code is for the Open port
> use
> >>>> only. There's no issue for the OS defines, but I wonder - just something
> >>>> to thing about - if TARGET_ARCH_xxx should be restored?
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>
> >>>>> My incremental changes are in this patch:
> >>>>> http://cr.openjdk.java.net/~goetz/wr16/8161259-
> >>>> newPfmIncl/webrev.03/incremental_changes.patch
> >>>>>
> >>>>> Best regards,
> >>>>> Goetz.
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>> Sent: Tuesday, July 19, 2016 3:37 AM
> >>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim Barrett
> >>>>>> <kim.barrett at oracle.com>
> >>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>>>> dev at openjdk.java.net
> >>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>>>
> >>>>>> Hi Goetz,
> >>>>>>
> >>>>>> On 18/07/2016 10:23 PM, David Holmes wrote:
> >>>>>>> On 18/07/2016 10:18 PM, Lindenmaier, Goetz wrote:
> >>>>>>>> Hi David,
> >>>>>>>>
> >>>>>>>> I'll introduce CPU_HEADER_H, but actually this should be fixed
> >>>>>>>> in the closed code.
> >>>>>>>
> >>>>>>> Bear with me, I'm trying to figure out how to do just that. :)
> >>>>>>>
> >>>>>>> The use of HOTSPOT_TARGET_CPU_DEFINE as the value for the
> ifdef
> >> is
> >>>> not
> >>>>>>> going to work for our closed ports, unless I change that value to
> >> match
> >>>>>>> the open naming scheme. But that in turn will lead to other
> problems
> >> as
> >>>>>>> we also need that value the way it is currently defined.
> >>>>>>>
> >>>>>>> I'll tackle this again in the morning when I'm fresher.
> >>>>>>
> >>>>>> Sorry but this really does need the CPU_HEADER_H macro. In
> general
> >> you
> >>>>>> can't just replace:
> >>>>>>
> >>>>>> #ifdef TARGET_ARCH_abc
> >>>>>>
> >>>>>> with
> >>>>>>
> >>>>>> #ifdef abc
> >>>>>>
> >>>>>> because the "abc"'s may not be mutually exclusive. In our case ARM
> >> and
> >>>>>> AARCH64 are both defined and are both needed. In contrast
> >>>>>> TARGET_ARCH_abc is uniquely defined as "abc" is chosen to
> represent
> >> the
> >>>>>> architecture in this context.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> David
> >>>>>>
> >>>>>>> Thanks,
> >>>>>>> David
> >>>>>>>
> >>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>> Goetz.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>>>> Sent: Montag, 18. Juli 2016 14:13
> >>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim
> Barrett
> >>>>>>>>> <kim.barrett at oracle.com>
> >>>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-
> >>>>>>>>> dev at openjdk.java.net
> >>>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>>>>>>
> >>>>>>>>> Hi Goetz,
> >>>>>>>>>
> >>>>>>>>> On 18/07/2016 8:21 PM, Lindenmaier, Goetz wrote:
> >>>>>>>>>> Hi David,
> >>>>>>>>>>
> >>>>>>>>>>>>> src/share/vm/prims/jni_md.h
> >>>>>>>>>>>>> src/share/vm/prims/jvm.h
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> // Can not use CPU_HEADER() macro, as it appends .hpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> can we not define a second form, eg CPU_HEADER_H, that
> >>>>>> appends.h
> >>>>>>>>>>>>> instead?
> >>>>>>>>>>>> I'll need one for OS, one for CPU, and each will be used only
> >> once.
> >>>>>>>>>>>> So I figured not to do it. But probably I should do it to get is
> >>>>>>>>>>>> similar everywhere.
> >>>>>>>>>>>
> >>>>>>>>>>> We actually need this as the simple arch names need not be
> >>>> mutually
> >>>>>>>>>>> exclusive - eg arm and aarch64. Otherwise this needs to be an
> if-
> >>>> else
> >>>>>>>>>>> construct in the correct order. ;-)
> >>>>>>>>>> They should be mutually exclusive, as they are set in
> >>>> CompileJvm.gmk
> >>>>>>>>>> in the same statement as the INCLUDE_SUFFIX_CPU:
> >>>>>>>>>> -D$(HOTSPOT_TARGET_CPU_DEFINE)
> >>>>>>>>>
> >>>>>>>>> Unfortunately we also have -DARM coming in from the closed
> part
> >> of
> >>>>>>>>> spec.gmk as we don't use AARCH64 for our port. So we get both
> >>>> defined
> >>>>>>>>> and try to include two platform files.
> >>>>>>>>>
> >>>>>>>>> Not sure how to try and resolve this yet. Trying to understand
> what
> >>>> the
> >>>>>>>>> role of HOTSPOT_TARGET_CPU_DEFINE is intended to be, as we
> >> are
> >>>> not
> >>>>>>>>> overriding it for our port - which I think is the current problem,
> but
> >>>>>>>>> changing it may break something else.
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>> David
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Best regards,
> >>>>>>>>>> Goetz
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>>>>>> Sent: Montag, 18. Juli 2016 11:06
> >>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Kim
> >> Barrett
> >>>>>>>>>>> <kim.barrett at oracle.com>
> >>>>>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-
> runtime-
> >>>>>>>>>>> dev at openjdk.java.net
> >>>>>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform files.
> >>>>>>>>>>>
> >>>>>>>>>>> On 18/07/2016 5:15 PM, Lindenmaier, Goetz wrote:
> >>>>>>>>>>>> Hi David,
> >>>>>>>>>>>>
> >>>>>>>>>>>> thanks for looking at all these files in more detail!
> >>>>>>>>>>>>
> >>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>>>>>>>> Sent: Montag, 18. Juli 2016 08:03
> >>>>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> Kim
> >>>> Barrett
> >>>>>>>>>>>>> <kim.barrett at oracle.com>
> >>>>>>>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-
> runtime-
> >>>>>>>>>>>>> dev at openjdk.java.net
> >>>>>>>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform
> files.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Goetz,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Again thanks for tackling this!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 15/07/2016 10:17 PM, Lindenmaier, Goetz wrote:
> >>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I made a new webrev:
> >>>>>>>>>>>>>> - '_' is added in makefile
> >>>>>>>>>>>>>> - uses Kim's macros
> >>>>>>>>>>>>>> - macros are capitalized
> >>>>>>>>>>>>>> - more comments in macros.hpp
> >>>>>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8161259-
> >>>>>>>>>>> newPfmIncl/webrev.02/
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Generally looks okay, a couple of clarifications and
> comments
> >>>>>> below.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> make/gensrc/GensrcAdlc.gmk
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> So the change from HOTSPOT_TARGET_CPU to
> >>>>>>>>>>>>> HOTSPOT_TARGET_CPU_ARCH only
> >>>>>>>>>>>>> affects the generated files right?
> >>>>>>>>>>>> Yes.
> >>>>>>>>>>>>> For our closed ports we still have
> >>>>>>>>>>>>> the _32/_64 source files in a common arch directory, but I
> >> don't
> >>>>>>>>>>>>> think
> >>>>>>>>>>>>> that needs to be reflected in the generated files. (Sorry I
> >>>>>>>>>>>>> haven't had
> >>>>>>>>>>>>> the time yet to apply this patch and see what needs to be
> >>>> changed
> >>>>>> on
> >>>>>>>>> the
> >>>>>>>>>>>>> closed side - but will start that once I send this :).)
> >>>>>>>>>>>> If I run plain configure, it generates a directory that has the
> >>>>>>>>>>>> word size
> >>>>>>>>>>>> in it's name:
> >>>>>>>>>>>> linux-x86_64-normal-server-release
> >>>>>>>>>>>> If I configure --with-target-bits=32, it builds to
> >>>>>>>>>>>> linux-x86-normal-server-release,
> >>>>>>>>>>>> so I figured this should be fine.
> >>>>>>>>>>>
> >>>>>>>>>>> Yes generated files are fine.
> >>>>>>>>>>>
> >>>>>>>>>>>>> make/lib/CompileJvm.gmk
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hmm but this change assumes no more _32/_64 header
> files if
> >> I
> >>>>>>>>>>>>> read it
> >>>>>>>>>>>>> right ?? So I'll need a common file that dispatches internally
> for
> >>>>>>>>>>>>> 32-bit and 64-bit.
> >>>>>>>>>>>> Yes.
> >>>>>>>>>>>> There is a single file in cpu/x86 where this did not hold:
> >>>>>>>>>>>> stubRoutines
> >>>>>>>>>>>> But this was rather small. And there anyways was a common
> file
> >>>>>>>>>>>> that was included in both.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> src/os/posix/vm/os_posix.cpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I'm wondering if we can use a similar trick to avoid this kind
> of
> >>>>>>>>>>>>> switch(OS) statement:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> #ifdef TARGET_OS_FAMILY_linux
> >>>>>>>>>>>>> Linux::ucontext_set_pc(ctx, pc);
> >>>>>>>>>>>>> #elif defined(TARGET_OS_FAMILY_solaris)
> >>>>>>>>>>>>> Solaris::ucontext_set_pc(ctx, pc);
> >>>>>>>>>>>>> ...
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ie something like:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> SUB(TARGET_OS)::ucontext_set_pc(ctx, pc);
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ? :)
> >>>>>>>>>>>> Hmm, I had to add the '_' to the string in TARGET_SO...
> >>>>>>>>>>>> Actually I think the implementation should be moved to
> >>>>>>>>>>>> os_linux.cpp/os_bsd.cpp etc.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> src/share/vm/code/nmethod.cpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Not clear why the platform include can simply be elided
> here ??
> >>>>>>>>>>>> It already includes code/nativeInst.hpp, which is the
> umbrella
> >>>> header
> >>>>>>>>>>>> for all the platform files.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> src/share/vm/jvmci/jvmciRuntime.cpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Shouldn't:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ! #endif // !LP64
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> be:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ! #endif // LP64
> >>>>>>>>>>>> Well, it ends the #else part ... but fixed anyways.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> src/share/vm/prims/jni_md.h
> >>>>>>>>>>>>> src/share/vm/prims/jvm.h
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> // Can not use CPU_HEADER() macro, as it appends .hpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> can we not define a second form, eg CPU_HEADER_H, that
> >>>>>> appends.h
> >>>>>>>>>>>>> instead?
> >>>>>>>>>>>> I'll need one for OS, one for CPU, and each will be used only
> >> once.
> >>>>>>>>>>>> So I figured not to do it. But probably I should do it to get is
> >>>>>>>>>>>> similar everywhere.
> >>>>>>>>>>>
> >>>>>>>>>>> We actually need this as the simple arch names need not be
> >>>> mutually
> >>>>>>>>>>> exclusive - eg arm and aarch64. Otherwise this needs to be an
> if-
> >>>> else
> >>>>>>>>>>> construct in the correct order. ;-)
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks,
> >>>>>>>>>>> David
> >>>>>>>>>>> -----
> >>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> src/share/vm/runtime/os.hpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Should we convert setjmp.h include to:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> #ifndef _WINDOWS
> >>>>>>>>>>>>> #include <setjmp.h>
> >>>>>>>>>>>>> #endif
> >>>>>>>>>>>>> #ifdef __APPLE__
> >>>>>>>>>>>>> ...
> >>>>>>>>>>>>> #endif
> >>>>>>>>>>>>> ?
> >>>>>>>>>>>> Fixed.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> BTW: why is it _WINDOWS instead of WINDOWS? Is that
> >> coming
> >>>>>> from
> >>>>>>>>> the
> >>>>>>>>>>>>> compiler itself?
> >>>>>>>>>>>> I wondered about that, too. Therefore I defined WINDOWS
> in
> >> the
> >>>>>>>>>>>> prototype webrev. Then I saw that our build is defining -
> >>>>>> D_WINDOWS
> >>>>>>>>>>> *and* -DWIN32,
> >>>>>>>>>>>> and removed it again using _WINDOWS instead.
> >>>>>>>>>>>> Eventually both should be replaced by WINDOWS, but not in
> this
> >>>>>>>>>>>> change.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> src/os_cpu/aix_ppc/vm/bytes_aix_ppc.inline.hpp
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Don't understand the point of this:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 28 #if defined(VM_LITTLE_ENDIAN)
> >>>>>>>>>>>>> 29 // Aix is not littel endian.
> >>>>>>>>>>>>> 30 #endif // VM_LITTLE_ENDIAN
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> also typo: littel
> >>>>>>>>>>>> I just copied the linux_ppc code and removed the unused
> >>>>>>>>> implementation.
> >>>>>>>>>>>> I wanted to document why there is this empty file.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Best regards,
> >>>>>>>>>>>> Goetz.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>> David
> >>>>>>>>>>>>> -----
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> Best regards,
> >>>>>>>>>>>>>> Goetz.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>>>>>>>>>> Sent: Freitag, 15. Juli 2016 11:11
> >>>>>>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> Kim
> >>>>>> Barrett
> >>>>>>>>>>>>>>> <kim.barrett at oracle.com>
> >>>>>>>>>>>>>>> Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-
> >> runtime-
> >>>>>>>>>>>>>>> dev at openjdk.java.net
> >>>>>>>>>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including platform
> >> files.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On 15/07/2016 5:30 PM, Lindenmaier, Goetz wrote:
> >>>>>>>>>>>>>>>> HI Kim,
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> thanks for this version of the macros, it's working on all
> the
> >>>>>>>>> platforms
> >>>>>>>>>>>>>>>> I can build.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Actually, I would prefer having the '_' in the macros and
> >> not
> >>>>>>>>>>>>>>>> on the
> >>>>>>>>>>>>>>>> command line. This way, parts of the name
> construction
> >> are
> >>>> in
> >>>>>>>>>>>>>>>> CompileJvm.gmk, other parts are in macros.hpp. But
> >>>>>> constructing
> >>>>>>>>>>>>>>>> the search path is also in the makefile, and uses the
> very
> >>>> same
> >>>>>>>>> string
> >>>>>>>>>>>>>>>> as the file suffixes. (Without '_', one could include that
> in
> >> the
> >>>>>>>>> macro,
> >>>>>>>>>>>>> too.)
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> But overall, I consider this a detail and am as fine with
> your
> >>>>>>>>>>>>>>>> solution
> >>>>>>>>>>>>>>>> as with the SUB() macros. What is better is that the
> linux=1
> >>>> etc
> >>>>>>>>>>>>>>>> problems are avoided.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Any more opinions whether the macros should be
> upper
> >> case?
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Yeah they probably should be.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>> David
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Best regards,
> >>>>>>>>>>>>>>>> Goetz.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>>>>>> From: Kim Barrett [mailto:kim.barrett at oracle.com]
> >>>>>>>>>>>>>>>>> Sent: Donnerstag, 14. Juli 2016 23:20
> >>>>>>>>>>>>>>>>> To: Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> >>>>>>>>>>>>>>>>> Cc: Andrew Haley <aph at redhat.com>; hotspot-
> compiler-
> >>>>>>>>>>>>>>>>> dev at openjdk.java.net; hotspot-runtime-
> >>>>>> dev at openjdk.java.net
> >>>>>>>>>>>>>>>>> Subject: Re: RFR(L): 8161259: Simplify including
> platform
> >>>> files.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> On Jul 14, 2016, at 3:18 PM, Lindenmaier, Goetz
> >>>>>>>>>>>>>>>>> <goetz.lindenmaier at sap.com> wrote:
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Hi everybody,
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> please take into account that these macros are only
> >> used
> >>>>>> within
> >>>>>>>>> 20
> >>>>>>>>>>>>> lines
> >>>>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>> the file macro.hpp. The code everybody needs to
> >>>> understand
> >>>>>> is
> >>>>>>>>>>>>>>>>>> #include cpu_header(bytes)
> >>>>>>>>>>>>>>>>>> which, in this example, is in file bytes.hpp and
> expands to
> >>>>>>>>>>>>>>>>> bytes_<cpu>.hpp.
> >>>>>>>>>>>>>>>>>> There are six of these, for cpu/os/os_cpu
> >>>> and .hpp/.inline.hpp.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> I really would appreciate if I don't have to spend days
> >>>>>>>>>>>>>>>>>> editing the
> >>>>>>>>>>>>>>>>>> 12 lines that use SUB().
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> @Kim
> >>>>>>>>>>>>>>>>>> I'm working on the s390 port, and posted my current
> >>>> progress
> >>>>>>>>>>> claiming
> >>>>>>>>>>>>>>>>>> that the biggest shared change I need to do is adding
> the
> >>>>>>>>> #includes.
> >>>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-
> >> dev/2016-
> >>>>>>>>>>>>>>>>> July/023782.html
> >>>>>>>>>>>>>>>>>> This arose the discussion about the includes.
> >>>>>>>>>>>>>>>>>> Later I posted a prototype of what Volker proposed
> in
> >> that
> >>>>>>>>>>> discussion
> >>>>>>>>>>>>>>>>>> to that thread:
> >>>>>>>>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-
> >> dev/2016-
> >>>>>>>>>>>>>>>>> July/023934.html
> >>>>>>>>>>>>>>>>>> which I now turned into a RFR.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Thanks.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I think I see what happened to the email thread for
> me; it
> >>>> looks
> >>>>>>>>> like
> >>>>>>>>>>> one
> >>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>> Andrew’s replies went to hotspot-compiler-dev but
> not
> >>>>>> hotspot-
> >>>>>>>>>>>>> runtime-
> >>>>>>>>>>>>>>>>> dev,
> >>>>>>>>>>>>>>>>> and I was not subscribed to the compiler list this
> morning.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I like the idea, just quibbling over details. I've only
> >>>>>>>>>>>>>>>>> reviewed
> >>>>>>>>>>>>>>>>> macros.hpp so far; the rest looks like it can wait until
> the
> >>>>>>>>>>>>>>>>> details
> >>>>>>>>>>>>>>>>> are settled.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> ------------------------------------------------------------------
> ----
> >> ---
> >>>> ---
> >>>>>> --
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> src/share/vm/utilities/macros.hpp
> >>>>>>>>>>>>>>>>> 470 // Helper macros to workaround existing #defines
> >> that
> >>>>>> spoil
> >>>>>>>>>>>>>>>>> 471 // the macro expansion. Detected so far: linux=1,
> >>>> sparc=1.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> This issue can be dodged by making the leading
> >> underscore
> >>>>>> part
> >>>>>>>>> of
> >>>>>>>>>>> the
> >>>>>>>>>>>>>>>>> expansion for INCLUDE_SUFFIX_*, e.g. in
> >>>>>>>>>>> make/lib/CompileJvm.gmk,
> >>>>>>>>>>>>> use
> >>>>>>>>>>>>>>>>> instead
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 66 JVM_CFLAGS_TARGET_DEFINES += \
> >>>>>>>>>>>>>>>>> 67 -
> DINCLUDE_SUFFIX_OS=_$(HOTSPOT_TARGET_OS)
> >> \
> >>>>>>>>>>>>>>>>> 68 -
> >>>>>>>>> DINCLUDE_SUFFIX_CPU=_$(HOTSPOT_TARGET_CPU_ARCH)
> >>>>>>>>>>> \
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Note the insertion of leading "_" for the values.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Then the whole macro block can be written as
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> #define cpu_header_stem(basename)
> >>>>>>>>> PASTE_TOKENS(basename,
> >>>>>>>>>>>>>>>>> INCLUDE_SUFFIX_CPU)
> >>>>>>>>>>>>>>>>> #define os_header_stem(basename)
> >>>>>> PASTE_TOKENS(basename,
> >>>>>>>>>>>>>>>>> INCLUDE_SUFFIX_OS)
> >>>>>>>>>>>>>>>>> #define os_cpu_header_stem(basename)
> >>>>>>>>>>> PASTE_TOKENS(basename,
> >>>>>>>>>>>>>>>>> PASTE_TOKENS(INCLUDE_SUFFIX_OS,
> >>>> INCLUDE_SUFFIX_CPU))
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> #define cpu_header(basename)
> >>>>>>>>>>>>>>> XSTR(cpu_header_stem(basename).hpp)
> >>>>>>>>>>>>>>>>> #define cpu_header_inline(basename)
> >>>>>>>>>>>>>>>>> XSTR(cpu_header_stem(basename).inline.hpp)
> >>>>>>>>>>>>>>>>> #define os_header(basename)
> >>>>>>>>>>>>> XSTR(os_header_stem(basename).hpp)
> >>>>>>>>>>>>>>>>> #define os_header_inline(basename)
> >>>>>>>>>>>>>>>>> XSTR(os_header_stem(basename).inline.hpp)
> >>>>>>>>>>>>>>>>> #define os_cpu_header(basename)
> >>>>>>>>>>>>>>>>> XSTR(os_cpu_header_stem(basename).hpp)
> >>>>>>>>>>>>>>>>> #define os_cpu_header_inline(basename)
> >>>>>>>>>>>>>>>>> XSTR(os_cpu_header_stem(basename).inline.hpp)
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> And SUB is no longer used...
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> If some future build system wants brackets instead of
> >>>>>>>>>>>>>>>>> strings, just
> >>>>>>>>>>>>>>>>> replace XSTR(...) with <...>.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> We lose if _linux or _sparc (for example) are defined,
> but
> >>>>>>>>>>>>>>>>> that's
> >>>>>>>>> true
> >>>>>>>>>>>>>>>>> for the webrev.01 code too. But note that underscore
> >>>>>> followed
> >>>>>>>>> by a
> >>>>>>>>>>>>>>>>> lowercase letter is not in the reserved word pattern
> for
> >>>> C/C++.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> BTW, my preference would be to use uppercase for
> the
> >>>> macro
> >>>>>>>>>>> names.
> >>>>>>>>>>>>> I
> >>>>>>>>>>>>>>>>> don't know what others think about that.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> ------------------------------------------------------------------
> ----
> >> ---
> >>>> ---
> >>>>>> --
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
More information about the hotspot-runtime-dev
mailing list