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

[Lois Foltan]

On 7/9/2014 8:53 AM, Lindenmaier, Goetz wrote:
> Hi Lois,
>
> thanks for looking at this change!
>
> In general, I did not replace xxx_<cpu>.hpp by the generic header,
> because it's a clear 1:1 relationship.  And as David says, if there is
> only the include cascade in the generic header, there is no point to
> including it.  Except that maybe including the generic files would
> be a  more consistent coding style.
> If it's agreed on, I'll fix this for all the headers I addressed.
> ... OK, in the meantime your other mail arrived ... so I'll leave it
> as is.
>
> The basic idea of .inline.hpp files is to avoid cycles when using inline
> functions.  A .inline.hpp file should never be included in a .hpp file, so there
> will never be a cycle.
>
> The VMRegImp:: functions in vmreg_<cpu>.inline.hpp  actually don't depend on
> other inline functions, so moving them to vmreg_<cpu>.hpp would be
> feasible. The XxxRegisterImpl:: functions must remain in a <cpu>.inline.hpp
> file, though.  Else we get a cycle between register.hpp and vmreg.hpp.
> If you want to, I move the code and rename the vmreg files to register...
> I think this would be a good cleanup, as currently the file contains
> implementations from two different headers, which is unusual.
>
> This is also why I placed the register.hpp include in vmreg.inline.hpp. It
> contains what actually should go to register.inline.hpp, so it should also contain
> the natural include for register.inline.hpp.

Hi Goetz,

Thank you for the further explanation with regards to vmreg.inline.hpp.  
I now understand why your included register.hpp in vmreg.inline.hpp but 
I still think it is unneccessary since vmreg.inline.hpp includes 
vmreg.hpp which includes register.hpp as well.

>
> stubGenerator_ppc.cpp:  The macro removed is declared in
> interp_masm_ppc.hpp, and I didn't want to include it there, seems
> not right.  So I rather removed the macro.

Ok, got it!

>
> I fixed the Copyright errors:
> http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/

Thank you.  Just a very minor nit, the copyright on the new file 
src/share/vm/opto/ad.hpp still seems incorrect.  It is "2000, 2014,".  
Shouldn't it be just "2014," since this is  a new file?

Overall I am good with your changes, reviewed!
Lois

>
> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: Lois Foltan [mailto:lois.foltan at oracle.com]
> Sent: Dienstag, 8. Juli 2014 19:42
> To: Lindenmaier, Goetz
> 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
>
> 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
>
> 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 ;)