s390x port progress: patch queue for hotspot assembled.

David Holmes david.holmes at oracle.com
Thu Jul 14 00:18:22 UTC 2016 

On 14/07/2016 1:30 AM, Coleen Phillimore wrote:
>
>
> Goetz,
>
> I like the cleanups in your patch but when I grep for TARGET_OS_FAMILY,
> I see this in jvm.cpp (which you changed) but also mutex.cpp and
> vmStructs.cpp which you didn't change.
>
> In mutex.cpp, this includes files that are all empty!

Time implement this cleanup:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-February/021617.html

:)

David

> #ifdef TARGET_OS_FAMILY_linux
> # include "mutex_linux.inline.hpp"
> #endif
> #ifdef TARGET_OS_FAMILY_solaris
> # include "mutex_solaris.inline.hpp"
> #endif
> #ifdef TARGET_OS_FAMILY_windows
> # include "mutex_windows.inline.hpp"
> #endif
> #ifdef TARGET_OS_FAMILY_bsd
> # include "mutex_bsd.inline.hpp"
> #endif
>
>
> Thanks,
> Coleen
>
> On 7/13/16 3:00 AM, Lindenmaier, Goetz wrote:
>> 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.
>
> I agree.
>> * Three of them are superfluous anyways.
>> 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.
>>
>>> 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.
>>
>> 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.
>>
>> 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
>>>>>>>>>>
>>>>>>>>>>
>

More information about the hotspot-dev mailing list