RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories

[David Holmes]

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 ;)
>>>>
>>