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