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

[Vladimir Kozlov]

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