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

[David Holmes]

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