RFR(L): 8161259: Simplify including platform files.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Jul 18 10:21:53 UTC 2016
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)
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