s390x port progress: patch queue for hotspot assembled.

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

On 13/07/2016 5:00 PM, 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.
> * 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 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>

More information about the hotspot-dev mailing list