s390x port progress: patch queue for hotspot assembled.

Thu Jul 14 08:31:31 UTC 2016

Hi David,

> >> Is there a reason you didn't do what I had suggested with the above and
> >> change the last _ to = ?
> > * I think the names should match what is in the path, therefore I think
> >    CPU is better than ARCH  (we have src/cpu/x86).
> > * I left out "FAMILY".  I don't see any gain in this part of the name.
> > * Three of them are superfluous anyways.
> 
> My view is that we should not change the names as they exist and thus
> avoid the inevitable debates over arch versus cpu verus whatever. It is
> messy enough that simply changing things again does not add any value in
> my opinion.

I think the matter of confusion is gone with my change.  There are no 
more includes that require the _64 ending, so there is no need to 
distinguish two categories of cpu or arch macros or whatever expressed
which processor the vm is compiled for.
Before there was
  -DTARGET_ARCH_MODEL_$(HOTSPOT_TARGET_CPU)
and
  -DTARGET_ARCH_$(HOTSPOT_TARGET_CPU_ARCH)
But now only one is left. 
Anyways, the old macros have been used strangely in a lot of 
places, so the name was misleading anyways. (See the cleanups
I have posted.)

I'll send a RFR soon.  I use 
   -DINCLUDE_SUFFIX_CPU=$(HOTSPOT_TARGET_CPU_ARCH)
in the webrev.

I wanted to reply directly on your mail, but maybe it's 
a good idea to continue the discussion in the RFR thread.

Best regards,
  Goetz.

> 
> > I thought about calling them INCLUDE_OS and INCLUDE_CPU, that
> > better indicates what they are meant for.
> > I also thought about only defining them in the macros.hpp file. We
> > have -DLINUX -DAMD64 / -DAIX -DPPC64 etc. on the command line
> > to identify which port to compile.
> 
> Not quite sure what you mean. These things should be being set by the
> build system for use in macros.hpp
> 
> >> I thought my suggestion would avoid the issue
> >> with the LINUX=1 problem. ?
> > Do you want me to do -DTARGET_OS_FAMILY=_linux ?  with '_'?
> > The problem is that there is lower case #define linux=1.
> > I don't think we should define things with leading '_', they are often
> > used by the system headers / compilers.
> 
> No I wasn't suggesting adding '_' - I thought we would avoid the issue.
> :( Often wondered why C lacks a means to say "don't macro-expand this"
> 
> 
> > I rebased my changes, that did not cause any conflicts. I also downloaded
> > the changesets and patched them into my repo, that works too.
> > You first need to apply the cleanups, then the new includes.
> 
> Ah I see.
> 
> Thanks,
> David
> 
> > Best regards,
> >   Goetz.
> >
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Mittwoch, 13. Juli 2016 05:43
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> >> 'dean.long at oracle.com' <dean.long at oracle.com>; hotspot-
> >> dev at openjdk.java.net
> >> Subject: Re: s390x port progress: patch queue for hotspot assembled.
> >>
> >> Hi Goetz,
> >>
> >> Thanks for doing this! I think it looks good. Only nit ...
> >>
> >> On 13/07/2016 7:58 AM, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> I implemented a prototype of the include model proposed by Volker.
> >>> I tested this so far on linux_x86_64, aix_ppc64 and linux_ppc64.
> >>> I'll test it on the other OSes/CPUs I have available tomorrow.
> >>>
> >>> The direcitves
> >>>   -DTARGET_OS_FAMILY_linux
> >>>   -DTARGET_ARCH_MODEL_x86_64
> >>>   -DTARGET_ARCH_x86
> >>>   -DTARGET_OS_ARCH_MODEL_linux_x86_64
> >>>   -DTARGET_OS_ARCH_linux_x86
> >>> are replaced by -DTARGET_OS=linux -DTARGET_CPU=x86.
> >>> (OS and CPU are used for vm_version.cpp)
> >>
> >> Is there a reason you didn't do what I had suggested with the above and
> >> change the last _ to = ? I thought my suggestion would avoid the issue
> >> with the LINUX=1 problem. ??
> >>
> >> I was going to test this out with our other platforms but unfortunately
> >> the patch didn't apply on latest jdk9/hs.
> >>
> >> Thanks,
> >> David
> >>
> >>> The macros proposed by Volker are in utilities/macros.hpp.
> >>>
> >>> There are some places where above TARGET_... macros are used for
> other
> >>> purposes than guarding the includes, I replaced them to use the other
> >>> platform dependent macros already defined.
> >>>
> >>> I also removed all headers carying _64 or _32 in their name.
> >>> There were three on ppc. As I learned there is no PPC32 port
> >>> any more, so changing this should be fine. I found one on x86
> >> (stubRoutines...hpp).
> >>> I merged it and use LP64 guards for the differences. I think this is
> >>> acceptable as the files are rather small and it's the only case.
> >>>
> >>> The adlc generated files all had suffix _64/_32 which I removed.
> >>> These are not significant, either.
> >>>
> >>> All this normalizing edits can be found in this webrev. It would be
> >>> useful to apply them anyways:
> >>> http://cr.openjdk.java.net/~goetz/wr16/newPfmIncl/webrev-cleanups/
> >>> The real change is here:
> >>> http://cr.openjdk.java.net/~goetz/wr16/newPfmIncl/webrev-
> >> newIncludes/
> >>> I had to do a tweak for the #define linux=1 in macros.hpp.
> >>>
> >>> Please have a look at these changes. Also, check whether this
> >>> works fine with the IDE of your choice :)
> >>>
> >>> If this finds appreciation, I'll open an Enhancement and make a
> >>> real webrev/RFR.
> >>>
> >>> Best regards,
> >>>   Goetz.
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net]
> On
> >>>> Behalf Of dean.long at oracle.com
> >>>> Sent: Wednesday, July 06, 2016 7:49 AM
> >>>> To: hotspot-dev at openjdk.java.net
> >>>> Subject: Re: s390x port progress: patch queue for hotspot assembled.
> >>>>
> >>>> On 7/5/16 10:42 PM, dean.long at oracle.com wrote:
> >>>>
> >>>>> On 7/4/16 2:59 PM, David Holmes wrote:
> >>>>>
> >>>>>> On 5/07/2016 7:44 AM, Stefan Karlsson wrote:
> >>>>>>> On 2016-07-04 23:03, David Holmes wrote:
> >>>>>>>> On 5/07/2016 12:07 AM, Volker Simonis wrote:
> >>>>>>>>> On Mon, Jul 4, 2016 at 3:17 PM, Andrew Haley
> <aph at redhat.com>
> >>>> wrote:
> >>>>>>>>>> On 04/07/16 12:22, David Holmes wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Unless the files are included into more than one other file
> then
> >>>>>>>>>>> adding
> >>>>>>>>>>> a level of indirection doesn't really help anything.
> >>>>>>>>>>
> >>>>>>>>>> I'm not sure what this means.
> >>>>>>>>>>
> >>>>>>>>>>> But I don't see why the platform part of the name needs to be
> >> kept
> >>>>>>>>>>> when
> >>>>>>>>>>> it is already part of the path?
> >>>>>>>>>>
> >>>>>>>>>> When switching between files in an editor, one doesn't usually
> >>>>>>>>>> type in the full path, but the filename.  I'd be very sad to lose
> >>>>>>>>>> that, and I think that unique filenames are a nice thing to have.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> In Emacs you can try the uniquify [1] extension which would
> solve
> >> the
> >>>>>>>>> problem. I already use it for editing various hotspot version (i.e.
> >>>>>>>>> hs-comp, jdk9-dev) in one Emacs. But I agree that Emacs is
> >> probably
> >>>>>>>>> not a practical solution for everybody :)
> >>>>>>>>>
> >>>>>>>>> [1]
> >>>>>>>>>
> >>>>
> >>
> https://www.gnu.org/software/emacs/manual/html_node/emacs/Uniquify.
> >>>> html
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Andrew.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> We could try something like this (adapted from [2]) although I'm
> >> not
> >>>>>>>>> sure if that's really a good solution (i.e. if it is treated right by
> >>>>>>>>> various IDEs):
> >>>>>>>>>
> >>>>>>>>> include_test.cpp
> >>>>>>>>> ============
> >>>>>>>>>
> >>>>>>>>> #define xstr(x) #x
> >>>>>>>>> #define str(x) xstr(x)
> >>>>>>>>> #define sub(x) x
> >>>>>>>>> #define cpu_header(x) str(sub(x)sub(_)CPU.hpp)
> >>>>>>>>> #define os_header(x) str(sub(x)sub(_)OS.hpp)
> >>>>>>>>> #define os_cpu_header(x)
> >> str(sub(x)sub(_)sub(OS)sub(_)CPU.hpp)
> >>>>>>>>>
> >>>>>>>>> #include cpu_header(assembler)
> >>>>>>>>> #include os_header(assembler)
> >>>>>>>>> #include os_cpu_header(assembler)
> >>>>>>>>
> >>>>>>>> This is the parameterization I thought was not possible. For some
> >>>>>>>> reason I thought #define'd values could not be used within
> #include
> >>>>>>>> directives.
> >>>>>>>
> >>>>>>> FWIW, we tried a similar approach when includeDB was removed,
> but
> >>>>>>> abandoned it because of the problem described below with the
> 'linux'
> >>>>>>> define.
> >>>>>>
> >>>>>> Give we have in the build (eg for linux x86) presently:
> >>>>>>
> >>>>>> -DTARGET_OS_FAMILY_linux
> >>>>>> -DTARGET_ARCH_MODEL_x86_32
> >>>>>> -DTARGET_ARCH_x86
> >>>>>> -DTARGET_OS_ARCH_MODEL_linux_x86_32
> >>>>>> -DTARGET_OS_ARCH_linux_x86
> >>>>>>
> >>>>>> I wonder if we can simply replace the above with:
> >>>>>>
> >>>>>> -DTARGET_OS_FAMILY=linux
> >>>>>> -DTARGET_ARCH_MODEL=x86_32
> >>>>>> -DTARGET_ARCH=x86
> >>>>>> -DTARGET_OS_ARCH_MODEL=linux_x86_32
> >>>>>> -DTARGET_OS_ARCH=linux_x86
> >>>>>>
> >>>>>> and use those variables for the include directives?
> >>>>>>
> >>>>>
> >>>>> I remember experimenting with that idea, but gave up because I
> >>>>> couldn't get it work.
> >>>>> What I ended up doing was something like this:
> >>>>>
> >>>>> #*include* C1_LIRGENERATOR_MD_HPP
> >>>>>
> >>>>
> >>>> Oops, let's try that again:
> >>>>
> >>>> #include C1_LIRGENERATOR_MD_HPP
> >>>>
> >>>> where, using x86 as an example, macros like
> C1_LIRGENERATOR_MD_HPP
> >>>> are
> >>>> defined in globalDefinitions_x86.hpp:
> >>>>
> >>>> #define C1_LIRGENERATOR_MD_HPP "c1_LIRGenerator_x86.hpp"
> >>>>
> >>>> dl
> >>>>
> >>>>> I don't know if this confuses IDEs or not.
> >>>>>
> >>>>> dl
> >>>>>
> >>>>>> David
> >>>>>>
> >>>>>>> StefanK
> >>>>>>>
> >>>>>>>>
> >>>>>>>> This looks very workable to me.
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> David
> >>>>>>>>
> >>>>>>>>> assembler_s390x.hpp
> >>>>>>>>> ================
> >>>>>>>>> #pragma message ("including \"assembler_s390x.h\"")
> >>>>>>>>>
> >>>>>>>>> assembler_linux.hpp
> >>>>>>>>> ===============
> >>>>>>>>> #pragma message ("including \"assembler_linux.h\"")
> >>>>>>>>>
> >>>>>>>>> assembler_linux_s390x.hpp
> >>>>>>>>> ====================
> >>>>>>>>> #pragma message ("including \"assembler_linux_s390x.h\"")
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> It works on all the platforms I have tested so far (see below)
> except
> >>>>>>>>> Linux where it needs some tweaks (see comments below):
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Solaris:
> >>>>>>>>> ======
> >>>>>>>>> /SS12u4/SUNWspro/bin/CC -DCPU=s390x -DOS="linux" -E
> >>>> include_test.cpp
> >>>>>>>>> #1 "include_test.cpp"
> >>>>>>>>> #1 "assembler_s390x.hpp"
> >>>>>>>>> #pragma message ( "including \"assembler_s390x.h\"" )
> >>>>>>>>> #1 "assembler_linux.hpp"
> >>>>>>>>> #pragma message ( "including \"assembler_linux.h\"" )
> >>>>>>>>> #1 "assembler_linux_s390x.hpp"
> >>>>>>>>> #pragma message ( "including \"assembler_linux_s390x.h\"" )
> >>>>>>>>>
> >>>>>>>>> AIX:
> >>>>>>>>> ===
> >>>>>>>>> /usr/vacpp_V12/usr/vacpp/bin/xlC_r -DCPU=s390x -DOS=linux -
> c
> >>>>>>>>> include_test.cpp
> >>>>>>>>> "assembler_s390x.hpp", line 1.9: 1540-1401 (I) An unknown
> >> "pragma
> >>>>>>>>> message" is specified.
> >>>>>>>>> "assembler_linux.hpp", line 1.9: 1540-1401 (I) An unknown
> "pragma
> >>>>>>>>> message" is specified.
> >>>>>>>>> "assembler_linux_s390x.hpp", line 1.9: 1540-1401 (I) An
> unknown
> >>>>>>>>> "pragma message" is specified.
> >>>>>>>>>
> >>>>>>>>> MacOS X
> >>>>>>>>> =======
> >>>>>>>>> /usr/bin/clang++ -DCPU=s390x -DOS=linux -c include_test.cpp
> >>>>>>>>> In file included from include_test.cpp:8:
> >>>>>>>>> ./assembler_s390x.hpp:1:9: warning: including
> >> "assembler_s390x.h"
> >>>>>>>>> [-W#pragma-messages]
> >>>>>>>>> #pragma message ("including \"assembler_s390x.h\"")
> >>>>>>>>>         ^
> >>>>>>>>> In file included from include_test.cpp:9:
> >>>>>>>>> ./assembler_linux.hpp:1:9: warning: including
> "assembler_linux.h"
> >>>>>>>>> [-W#pragma-messages]
> >>>>>>>>> #pragma message ("including \"assembler_linux.h\"")
> >>>>>>>>>         ^
> >>>>>>>>> In file included from include_test.cpp:10:
> >>>>>>>>> ./assembler_linux_s390x.hpp:1:9: warning: including
> >>>>>>>>> "assembler_linux_s390x.h" [-W#pragma-messages]
> >>>>>>>>> #pragma message ("including \"assembler_linux_s390x.h\"")
> >>>>>>>>>         ^
> >>>>>>>>> 3 warnings generated.
> >>>>>>>>>
> >>>>>>>>> Windows:
> >>>>>>>>> =======
> >>>>>>>>> $ /cygdrive/c/progra~2/micros~1.0/vc/bin/amd64/cl -
> DCPU=s390x
> >>>>>>>>> -DOS=linux -c include_test.
> >>>>>>>>> cpp
> >>>>>>>>> Microsoft (R) C/C++ Optimizing Compiler Version 16.00.40219.01
> for
> >>>>>>>>> x64
> >>>>>>>>> Copyright (C) Microsoft Corporation.  All rights reserved.
> >>>>>>>>>
> >>>>>>>>> include_test.cpp
> >>>>>>>>> including "assembler_s390x.h"
> >>>>>>>>> including "assembler_linux.h"
> >>>>>>>>> including "assembler_linux_s390x.h"
> >>>>>>>>>
> >>>>>>>>> HPUX:
> >>>>>>>>> =====
> >>>>>>>>> $ /opt/aCC/bin/aCC -DCPU=s390x -DOS=linux -c include_test.cpp
> >>>>>>>>> Info #4087-D: "including \"assembler_s390x.h\""
> >>>>>>>>>
> >>>>>>>>> Info #4087-D: "including \"assembler_linux.h\""
> >>>>>>>>>
> >>>>>>>>> Info #4087-D: "including \"assembler_linux_s390x.h\""
> >>>>>>>>>
> >>>>>>>>> Linux:
> >>>>>>>>> =====
> >>>>>>>>> g++ -DCPU=s390x -DOS=linux -c include_test.cpp
> >>>>>>>>> include_test.cpp:9:30: fatal error: assembler_1.hpp: No such file
> or
> >>>>>>>>> directory
> >>>>>>>>> compilation terminated.
> >>>>>>>>>
> >>>>>>>>> The problem on Linux is that 'linux' is already predefined to '1'
> but
> >>>>>>>>> that could be easily fixed for example by slightly changing the
> name
> >>>>>>>>> or by adding the underscore to the definiton of the OS macro
> (i.e.
> >>>>>>>>> -DOS=_linux).
> >>>>>>>>>
> >>>>>>>>> As I said, I'm still not sure if this looks nice? I just wanted to
> >>>>>>>>> post my findings for discussion :)
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Volker
> >>>>>>>>>
> >>>>>>>>> [2]
> >>>>>>>>> http://stackoverflow.com/questions/1852652/how-can-i-
> include-
> >> a-
> >>>> file-whose-name-is-built-up-from-a-macro
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>
> >>>