Request for reviews (M): 6677625: Move platform specific flags from globals.hpp to globals_<arch>.hpp

[Coleen Phillimore]

On 8/27/2012 1:00 PM, Tao Mao wrote:
> Thank you David. This is my first project.

It looks very good!
Coleen

>
> Tao
>
> On 8/26/2012 5:56 PM, David Holmes wrote:
>> On 25/08/2012 7:55 AM, Christian Thalinger wrote:
>>> Tao reworked his changes.  It's much cleaner now:
>>>
>>> http://cr.openjdk.java.net/~twisti/6677625/
>>
>> Much, much cleaner!
>>
>> Thumbs up from me.
>>
>> David
>>
>>> -- Chris
>>>
>>> On Aug 22, 2012, at 5:36 AM, Coleen 
>>> Phillimore<coleen.phillimore at oracle.com>  wrote:
>>>
>>>> On 8/21/2012 9:29 PM, David Holmes wrote:
>>>>> I'm happy with moving the macros to the top of globals.hpp if that 
>>>>> simplifies things.
>>>>
>>>> Yes, I agree.  This change looks great otherwise!
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 22/08/2012 11:22 AM, Vladimir Kozlov wrote:
>>>>>> David Holmes wrote:
>>>>>>> On 22/08/2012 8:47 AM, Vladimir Kozlov wrote:
>>>>>>>> David Holmes wrote:
>>>>>>>>> I see a lot of changes that aren't obviously pd flags. Is 
>>>>>>>>> there some
>>>>>>>>> other refactoring going on here?
>>>>>>>>
>>>>>>>> In addition to flags which were used only on one platforms some C2
>>>>>>>> specific flags were moved into opto/c2_globals.hpp. And some 
>>>>>>>> unused
>>>>>>>> flags were removed.
>>>>>>>
>>>>>>> Ok. But the change in src/share/vm/opto/runtime.cpp seems 
>>>>>>> unrelated to
>>>>>>> this work.
>>>>>>
>>>>>> It is C2 runtime (wrappers for calls into VM runtime).
>>>>>> That code was always dead and never used (from some early stage 
>>>>>> of C2
>>>>>> development). It was only a placed where
>>>>>> UpdateHotSpotCompilerFileOnError flag was used. So we remove the 
>>>>>> flag
>>>>>> and the code.
>>>>>>
>>>>>>>
>>>>>>> I also think the macros for globals were better placed in 
>>>>>>> globals.hpp
>>>>>>> rather than being moved to macros.hpp.
>>>>>>
>>>>>> The problem was that these macros were defined at the bottom of
>>>>>> globals.hpp so they can't be used in platform specific *.hpp 
>>>>>> which are
>>>>>> included at the beginning of globals.hpp. But we can still keep
>>>>>> definitions in globals.hpp if we move them to the top instead of
>>>>>> #include "utilities/macros.hpp".
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>>>>> What happens with these changes? Unrecognized VM option? That 
>>>>>>>>> could
>>>>>>>>> break people's scripts.
>>>>>>>>
>>>>>>>> Yes, they will be unrecognized. There is flag to avoid that and 
>>>>>>>> our SQE
>>>>>>>> is using it already: -XX:+IgnoreUnrecognizedVMOptions
>>>>>>>>
>>>>>>>> And we [re]moved flags which should not be used by general 
>>>>>>>> public, they
>>>>>>>> are our internal flags.
>>>>>>>
>>>>>>> Well I didn't catalog all those changed but you know that the 
>>>>>>> above is
>>>>>>> not true in general. -XX flags are used a lot in places they
>>>>>>> "shouldn't". :( Hopefully the PD ones are less likely to be used.
>>>>>>
>>>>>> I agree with your about -XX general flags. We tried to not touch 
>>>>>> those.
>>>>>>
>>>>>>>
>>>>>>> Overall I don't understand the form of this change. I would 
>>>>>>> expect all
>>>>>>> the platform specific stuff to be in the hpp files included by the
>>>>>>> shared cpp file. All these new cpp files seem somewhat redundant 
>>>>>>> to me.
>>>>>>
>>>>>> You are right.
>>>>>>
>>>>>> We followed C2_FLAGS definitions and started with 
>>>>>> X86_FLAGS/SPARC_FLAGS.
>>>>>> But then we realized that ARCH_FLAGS could be general since it 
>>>>>> should be
>>>>>> present on all platforms. And next step, as you said, we may be able
>>>>>> also move MATERIALIZE code into globals.cpp instead of creating new
>>>>>> platform specific .cpp files.
>>>>>>
>>>>>> I will ask Tao to try it.
>>>>>>
>>>>>>> And what does an ARCH_PD_ flag mean? I couldn't see any examples of
>>>>>>> them. Something different between x86-solaris and x86-linux for 
>>>>>>> example?
>>>>>>>
>>>>>>> And why do we bracket things with identical comments eg:
>>>>>>
>>>>>> We can remove all that if you agree to move macros definitions to 
>>>>>> the
>>>>>> top of globals.hpp.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> ! /* all DECLARE_*_FLAG 's are defined in utilities/macros.hpp */
>>>>>>> RUNTIME_FLAGS(DECLARE_DEVELOPER_FLAG, DECLARE_PD_DEVELOPER_FLAG, 
>>>>>>> ...
>>>>>>>
>>>>>>> RUNTIME_OS_FLAGS(DECLARE_DEVELOPER_FLAG, 
>>>>>>> DECLARE_PD_DEVELOPER_FLAG, ...
>>>>>>> + /* all DECLARE_*_FLAG 's are defined in utilities/macros.hpp */
>>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>> On 22/08/2012 7:23 AM, Christian Thalinger wrote:
>>>>>>>>>> http://cr.openjdk.java.net/~twisti/6677625/
>>>>>>>>>>
>>>>>>>>>> 6677625: Move platform specific flags from globals.hpp to
>>>>>>>>>> globals_<arch>.hpp
>>>>>>>>>> Reviewed-by:
>>>>>>>>>> Contributed-by: Tao Mao<tao.mao at oracle.com>
>>>>>>>>>>
>>>>>>>>>> Add new definition RUNTIME_PD_FLAGS and move platform 
>>>>>>>>>> specific flags
>>>>>>>>>> from
>>>>>>>>>> globals.hpp to globals_<arch>.hpp.
>>>>>>>>>>
>>>>>>>>>> src/cpu/sparc/vm/globals_sparc.cpp
>>>>>>>>>> src/cpu/sparc/vm/globals_sparc.hpp
>>>>>>>>>> src/cpu/x86/vm/globals_x86.cpp
>>>>>>>>>> src/cpu/x86/vm/globals_x86.hpp
>>>>>>>>>> src/cpu/zero/vm/globals_zero.cpp
>>>>>>>>>> src/cpu/zero/vm/globals_zero.hpp
>>>>>>>>>> src/share/vm/c1/c1_globals.hpp
>>>>>>>>>> src/share/vm/opto/c2_globals.cpp
>>>>>>>>>> src/share/vm/opto/c2_globals.hpp
>>>>>>>>>> src/share/vm/opto/runtime.cpp
>>>>>>>>>> src/share/vm/runtime/globals.cpp
>>>>>>>>>> src/share/vm/runtime/globals_extension.hpp
>>>>>>>>>> src/share/vm/runtime/globals.hpp
>>>>>>>>>> src/share/vm/utilities/macros.hpp
>>>>>>>>>>
>>>>
>>>