RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Jul 15 09:18:28 UTC 2014
Hi David,
There are no clean rules followed, which happens to cause
compile problems here and there. I try to clean this up a bit.
If inline function foo() calls another inline function bar(), the c++ compiler
must see both implementations to compile foo (else it obviously can't
inline). It must see the declaration of the function to be inlined before
the function where it is inlined. If there are cyclic inlines you need inline.hpp
headers to get a safe state. Also, to be on the safe side, .hpp files never may include
.inline.hpp files, else an implementation can end up above the declaration
it needs. See also the two examples attached.
If there is no cycle, it doesn't matter. That's why a lot of functions
are not placed according to this scheme.
For the functions I moved to the header (path_separator etc):
They are used in a lot of .hpp files. Moving them to os.hpp I easily could avoid
including the os.inline.hpp in .hpp files, which would be bad.
Best regards,
Goetz.
-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com]
Sent: Dienstag, 15. Juli 2014 09:20
To: Lindenmaier, Goetz; Coleen Phillimore; hotspot-dev at openjdk.java.net
Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
On 15/07/2014 4:34 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> functions that are completely self contained can go into the .hpp.
> Functions that call another inline function defined in an other header
> must go to .inline.hpp as else there could be cycles the c++ compilers can't
> deal with.
A quick survey of the shared *.inline.hpp files shows many don't seem to
fit this definition. Are templates also something that needs special
handling?
I'm not saying anything is wrong with your changes, just trying to
understand what the rules are.
Thanks,
David
> Best regards,
> Goetz.
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 15. Juli 2014 00:26
> To: Lindenmaier, Goetz; Coleen Phillimore; hotspot-dev at openjdk.java.net
> Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
>
> On 14/07/2014 10:37 PM, Lindenmaier, Goetz wrote:
>> Hi Coleen,
>>
>> Thanks for sponsoring this!
>>
>> bytes, ad, nativeInst and vmreg.inline were used quite often
>> in shared files, so it definitely makes sense for these to have
>> a shared header.
>> vm_version and register had an umbrella header, but that
>> was not used everywhere, so I cleaned it up.
>> That left adGlobals, jniTypes and interp_masm which
>> are only used a few time. I did these so that all files
>> are treated similarly.
>> In the end, I didn't need a header for all, as they were
>> not really needed in the shared files, or I found
>> another good place, as for adGlobals.
>>
>> I added you and David H. as reviewer to the webrev:
>> http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/
>> I hope this is ok with you, David.
>
> It might be somewhat premature :) I somewhat confused by the rules for
> headers and includes and inlines. I now see with this change a bunch of
> inline function definitions being moved out of the .inline.hpp file and
> into the .hpp file. Why? What criteria determines if an inline function
> goes into the .hpp versus the .inline.hpp file ???
>
> Thanks,
> David
>
>> Thanks,
>> Goetz.
>>
>>
>> -----Original Message-----
>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
>> Sent: Montag, 14. Juli 2014 14:09
>> To: hotspot-dev at openjdk.java.net
>> Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
>>
>>
>> I think this looks like a good cleanup. I can sponsor it and make the
>> closed changes also again. I initially proposed the #include cascades
>> because the alternative at the time was to blindly create a dispatching
>> header file for each target dependent file. I wanted to see the
>> #includes cleaned up instead and target dependent files included
>> directly. This adds 5 dispatching header files, which is fine. I
>> think the case of interp_masm.hpp is interesting though, because the
>> dispatching file is included in cpu dependent files, which could
>> directly include the cpu version. But there are 3 platform independent
>> files that include it. I'm not going to object though because I'm
>> grateful for this cleanup and I guess it's a matter of opinion which is
>> best to include in the cpu dependent directories.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 7/14/14, 3:56 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> David, can I consider this a review?
>>>
>>> And I please need a sponsor for this change. Could somebody
>>> please help here? Probably some closed adaptions are needed.
>>> It applies to any repo as my other change traveled around
>>> by now.
>>>
>>> Thanks and best regards,
>>> Goetz.
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Freitag, 11. Juli 2014 07:19
>>> To: Lindenmaier, Goetz; Lois Foltan
>>> Cc: hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
>>>
>>> On 10/07/2014 12:03 AM, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> foo.hpp as few includes as possible, to avoid cycles.
>>>> foo.inline.hpp * must have foo.hpp, as it contains functions declared in foo.hpp
>>>> (either directly or via the platform files.)
>>>> * should include foo.platform.inline.hpp, so that shared files that
>>>> call functions from foo.platform.inline.hpp need not contain the
>>>> cascade of all the platform files.
>>>> If code in foo.platform.inline.hpp is only used in the platform files,
>>>> it is not necessary to have an umbrella header.
>>>> foo.platform.inline.hpp Should include what is needed in its code.
>>>>
>>>> For client code:
>>>> With this change I now removed all include cascades of platform files except for
>>>> those in the 'natural' headers.
>>>> Shared files, or files with less 'diversity' should include the general foo.[inline.]hpp.
>>>> (foo.platform.cpp should not get a cascade with bar.os_platform.[inline.]hpp
>>>> headers, but include bar.[inline.]hpp.)
>>>> If it's 1:1, I don't care, as discussed before.
>>>>
>>>> Does this make sense?
>>> I find the overall structure somewhat counter-intuitive from an
>>> implementation versus interface perspective. But ...
>>>
>>> Thanks for the explanation.
>>>
>>> David
>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>> which of the above should #include which others, and which should be
>>>> #include'd by "client" code?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> src/cpu/sparc/vm/c1_Runtime1_sparc.cpp
>>>>>>> - include nativeInst.hpp instead of nativeInst_sparc.hpp
>>>>>>> - include vmreg.inline.hpp instead of vmreg_sparc.inline.hpp
>>>>>>> (however this could pull in more code than needed since
>>>>>>> vmreg.inline.hpp also includes asm/register.hpp and code/vmreg.hpp)
>>>>>>>
>>>>>>> src/cpu/ppc/vm/stubGenerator_ppc.cpp
>>>>>>> - change not related to clean up of umbrella headers, please
>>>>>>> explain/justify.
>>>>>>>
>>>>>>> src/share/vm/code/vmreg.hpp
>>>>>>> - Can lines #143-#15 be replaced by an inclusion of
>>>>>>> vmreg.inline.hpp or will
>>>>>>> this introduce a cyclical inclusion situation, since
>>>>>>> vmreg.inline.hpp includes vmreg.hpp?
>>>>>>>
>>>>>>> src/share/vm/classfile/classFileStream.cpp
>>>>>>> - only has a copyright change in the file, no other changes
>>>>>>> present?
>>>>>>>
>>>>>>> src/share/vm/prims/jvmtiClassFileReconstituter.cpp
>>>>>>> - incorrect copyright, no current year?
>>>>>>>
>>>>>>> src/share/vm/opto/ad.hpp
>>>>>>> - incorrect copyright date for a new file
>>>>>>>
>>>>>>> src/share/vm/code/vmreg.inline.hpp
>>>>>>> - technically this new file does not need to include
>>>>>>> "asm/register.hpp" since
>>>>>>> vmreg.hpp already includes it
>>>>>>>
>>>>>>> My only lingering concern is the cyclical nature of
>>>>>>> vmreg.hpp/vmreg.inline.hpp. It might be better to not introduce the new
>>>>>>> file "vmreg.inline.hpp" in favor of having files include vmreg.hpp
>>>>>>> instead? Again since vmreg.inline.hpp includes vmreg.hpp there really
>>>>>>> is not much difference between the two?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Lois
>>>>>>>
>>>>>>>
>>>>>>> On 7/7/2014 4:52 AM, Lindenmaier, Goetz wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I decided to clean up the remaining include cascades, too.
>>>>>>>>
>>>>>>>> This change introduces umbrella headers for the files in the cpu
>>>>>>>> subdirectories:
>>>>>>>>
>>>>>>>> src/share/vm/utilities/bytes.hpp
>>>>>>>> src/share/vm/opto/ad.hpp
>>>>>>>> src/share/vm/code/nativeInst.hpp
>>>>>>>> src/share/vm/code/vmreg.inline.hpp
>>>>>>>> src/share/vm/interpreter/interp_masm.hpp
>>>>>>>>
>>>>>>>> It also cleans up the include cascades for adGlobals*.hpp,
>>>>>>>> jniTypes*.hpp, vm_version*.hpp and register*.hpp.
>>>>>>>>
>>>>>>>> Where possible, this change avoids includes in headers.
>>>>>>>> Eventually it adds a forward declaration.
>>>>>>>>
>>>>>>>> vmreg_<cpu>.inline.hpp contains functions declared in register_cpu.hpp
>>>>>>>> and vmreg.hpp, so there is no obvious mapping to the shared files.
>>>>>>>> Still, I did not split the files in the cpu directories, as they are
>>>>>>>> rather small.
>>>>>>>>
>>>>>>>> I didn't introduce a file for adGlobals_<cpu>.hpp, as adGlobals mainly
>>>>>>>> contains machine dependent, c2 specific register information. So I
>>>>>>>> think optoreg.hpp is a good header to place the adGlobals_<cpu>.hpp
>>>>>>>> includes in,
>>>>>>>> and then use optoreg.hpp where symbols from adGlobals are needed.
>>>>>>>>
>>>>>>>> I moved the constructor and destructor of CodeletMark to the .cpp
>>>>>>>> file, I don't think this is performance relevant. But having them in
>>>>>>>> the header requirs to pull interp_masm.hpp into interpreter.hpp, and
>>>>>>>> thus all the assembler include headers into a lot of files.
>>>>>>>>
>>>>>>>> Please review and test this change. I please need a sponsor.
>>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/
>>>>>>>>
>>>>>>>> I compiled and tested this without precompiled headers on linuxx86_64,
>>>>>>>> linuxppc64,
>>>>>>>> windowsx86_64, solaris_sparc64, solaris_sparc32, darwinx86_64,
>>>>>>>> aixppc64, ntamd64
>>>>>>>> in opt, dbg and fastdbg versions.
>>>>>>>>
>>>>>>>> Currently, the change applies to hs-rt, but once my other change
>>>>>>>> arrives in other
>>>>>>>> repos, it will work there, too. (I tested it together with the other
>>>>>>>> change
>>>>>>>> against jdk9/dev, too.)
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Goetz.
>>>>>>>>
>>>>>>>> PS: I also did all the Copyright adaptions ;)
>>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: test.cpp
URL: <http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20140715/4fe4f287/test-0001.cpp>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: test2.cpp
URL: <http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20140715/4fe4f287/test2-0001.cpp>
More information about the hotspot-dev
mailing list