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

[Vladimir Kozlov]

Thank oyu, David

Vladimir

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