RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jul 15 13:57:01 UTC 2014
It also seems to me that these vmreg_ppc.hpp inline functions are
special in that they are included directly in the class declaration,
rather than the preferred separate class declaration. So I think this
doesn't follow the "rules" as such because this case is different. It
would be nice to clean out these includes in another cleanup pass.
I hit the same cycles on the closed part but didn't realize it was
because of cycles.
Thanks,
Coleen
On 7/15/14, 5:18 AM, Lindenmaier, Goetz wrote:
> 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 ;)
More information about the hotspot-dev
mailing list