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

[David Holmes]

I'm happy with moving the macros to the top of globals.hpp if that 
simplifies things.

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