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

[Volker Simonis]

On Wed, Jul 9, 2014 at 11:31 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>
> 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.
>

I think it is good practice that every file includes all the other
files it depends on an not relies on the fact that some dependencies
are included indirectly. In the given example, removing register.hpp
from vmreg.hpp (for whatever reason) would break vmreg.inline.hpp as a
side effect.

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