RFR(L): 8161259: Simplify including platform files.

David Holmes david.holmes at oracle.com
Tue Jul 19 01:37:27 UTC 2016 

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