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

Tue Jul 19 05:58:51 UTC 2016

Hi Coleen, 

thanks for looking at this.
> http://cr.openjdk.java.net/~goetz/wr16/8161259-
> newPfmIncl/webrev.02/src/cpu/aarch64/vm/vm_version_aarch64.cpp.udiff.
> html
> +#include OS_HEADER(os) 
> Why isn't this OS_HEADER_INLINE (if there's such a macro), since it's
> including the .inline.hpp file?
Good catch, fixed.

> Why is there just this single #include os_linux.inline.hpp and not the
> cascade of os_aix.hpp files?
Which file do you mean?

> For the record, the original intent on having these cascading #include
> directives and not having dispatch .hpp files (ala creating an
> "os_cpu.hpp" file for example) was so that eventually all the #include
> directives would be pushed down to the lowest level that needed it, and
> everything would be super-local and clean :)   For example, the only cpp
> files that needed anything in assembler_x86.hpp would be already located
> in the src/cpu/x86/vm directory and could include assembler_x86.hpp
> directly.   I still think this is a worthwhile goal but from reading
> this, the sources are far from this.
This might be nice, but the platform files often implement a inline method called
in shared code, so the shared code must include them. For the macroAssembler
example: 
macroAssembler_sparc.inline.hpp defines  
MacroAssembler::bang_stack_with_offset(int offset)
which is called in assembler.cpp.

Probably this can be cleaned up for the assembler files (I don't think inlining
 this method has any performance effects, and other similar cases should 
not either), but it won't be possible for all.  Especially the atomic headers
from the os_cpu directories are needed as inline functions.

But with the new macros pure umbrella headers could be removed.
I still find them quite useful.

> I also didn't want us to have to create empty platform files (but I see
> some were added here).   Maybe macroAssembler_cpu.inline.hpp inclusion
> can be pushed down after this change as a further cleanup.
Actually, I'm removing more empty files than I add :). But it's a fact that these
macros require them, except if I surround them by #ifndef X86 (for this case).

> This looks great though, and sorely needed, especially when adding a new
> platform or cpu implementation.
Yes, we should have had this before the ppc port, would have saved work 
for us and aarch :)  But it really simplifies porting!

Best regards,
  Goetz.

> 
> Thanks,
> Coleen
> 
> On 7/15/16 8:17 AM, 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/
> >
> > 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.
> >>>>
> >>>> ------------------------------------------------------------------------------