s390x port progress: patch queue for hotspot assembled.

Coleen Phillimore coleen.phillimore at oracle.com
Wed Jul 13 15:30:25 UTC 2016 


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!

#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