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

[Lois Foltan]

On 7/8/2014 10:23 PM, David Holmes wrote:
> Hi Lois,
>
> On 9/07/2014 3:42 AM, Lois Foltan wrote:
>> Hi Goetz,
>>
>> Overall this cleanup looks good.  Here are specific comments per file:
>>
>> src/cpu/ppc/vm/runtime_ppc.cpp
>>      - include nativeInst.hpp instead of nativeInst_ppc.hpp
>
> Hmmm - doesn't this go against the argument Coleen was making with 
> regard to the other umbrella header situation? She said a platform 
> specific file should include the platform specific header rather than 
> the generic top-level header.
>
> I must admit I'm not completely convinced as it depends on whether the 
> platform specific implementation calls generic functions that may or 
> may not have a platform specific implementation.

Hi David,

Yes, you are correct, I looked back to some of the email discussion from 
JDK-8048241.  I share your thoughts on this topic but will defer since 
this was a recent discussion.  Goetz, please disregard my comments about 
including <file>.hpp instead of <file>_platform.hpp below.  I think this 
just affected src/cpu/ppc/vm/runtime_ppc.cpp and 
src/cpu/sparc/vm/c1_Runtime1_sparc.cpp.

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