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

Tue Jul 19 07:47:32 UTC 2016

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)

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.
> >>>>>>>>>>>
> >>>>>>>>>>> ----------------------------------------------------------------------------
> --
> >>>>>>>>>>>
> >>>>>>>>>>