RFR(L): 8161259: Simplify including platform files.
David Holmes
david.holmes at oracle.com
Wed Jul 20 10:04:36 UTC 2016
On 20/07/2016 7:21 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> OK, to get this through I will add
> -DTARGET_ARCH_$(HOTSPOT_TARGET_CPU_ARCH)
> and revert this one and only place it needs to be used in the vmStructs_jvmci.cpp.
Thanks.
> For the records, openJdk aarch64 has a C1 port. And yes, such cleanups should
> not be in this change.
I did not know they had a 64-bit C1 - interesting.
>
> Thanks for doing the closed changes!
You're welcome.
Cheers,
David
> Best regards,
> Goetz.
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Mittwoch, 20. Juli 2016 11: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 20/07/2016 6:47 PM, Lindenmaier, Goetz wrote:
>>> 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 would not want to do this and certainly not as part of this change.
>> If/when the Aarch32 port arrives we may have to revisit this, but not
>> right now, please.
>>
>>> 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?
>>
>> Yes - it refers to specific piece of code in the open aarch64 sources.
>>
>>> (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?
>>
>> I don't think the open port has C1 so it would ignore the above files
>> anyway.
>>
>>> 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.
>>
>> I really do not want to make any changes to this - ignoring the include
>> macro changes everything we have is working perfectly fine just the way
>> the defines are. So I don't want to see this change potentially break
>> that through incidental changes.
>>
>> I do not see having the following is a big issue:
>>
>> -DTARGET_ARCH_$(HOTSPOT_TARGET_CPU_ARCH)
>> -DINCLUDE_SUFFIX_CPU=_$(HOTSPOT_TARGET_CPU_ARCH)
>>
>> It allows TARGET_ARCH_aarch64 to mean the open ARMv8 port, and
>> TARGET_ARCH_arm to mean whatever the owners of that define intend it to
>> mean. It certainly is a lot better than interpreting what the
>> combinations of AARCH64 and ARM mean. Keeping this removes the need to
>> perform some of the changes as noted above.
>>
>> I'm preparing the review of the internal changes we need to accompany this.
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> 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-compiler-dev
mailing list