[13] RFR: 8213416: Replace some enums with static const members in hotspot/compiler
Rahul Raghavan
rahul.v.raghavan at oracle.com
Wed May 15 14:46:32 UTC 2019
Hi,
Request help review and finalize fix for 8213416.
<weberev.00> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8213416
The requirement is to solve
"enumeral and non-enumeral type in conditional expression" warnings
triggered with -Wextra enabled for gcc on hotspot.
(hotspot/compiler part is handled in this 8213416 task
and hotspot/runtime in 8223400)
The same warning is generated for ternary operator statements like-
(x ? int_val : enum_val).
e.g.:
comp_level = TieredCompilation ? TieredStopAtLevel : CompLevel_highest_tier;
Understood from comments that the following type typecast solution
proposed earlier was not accepted.
- comp_level = TieredCompilation ? TieredStopAtLevel :
CompLevel_highest_tier;
+ comp_level = TieredCompilation ? TieredStopAtLevel : (int)
CompLevel_highest_tier;
and then proposed solution was to rewrite those enums to be static const
members.
Tried changes based on the comments info from JBS.
Extracts of related JBS comments-
- ".... it's just a simple code refactoring. "
- "David H. only complained about NO_HASH which we can fix.
We can also fix CompLevel_highest_tier usage - should use CompLevel type
everywhere.
But I would not touch Op_RegFlags -
I don't want to complicate its construction and
we have a lot of places where Op_<reg> are used as uint.
I would only fix places where it is used as int to make sure it is
used as uint everywhere."
Reported enums in question for hotspot/compiler
1) NO_HASH
2) CompLevel_highest_tier
3) Op_RegFlags
4) _lh_array_tag_obj_value, _lh_instance_slow_path_bit
1) NO_HASH
tried [open/src/hotspot/share/opto/node.hpp]
- enum { NO_HASH = 0 };
+ static const uint NO_HASH = 0;
2) CompLevel_highest_tier
Only one warning in process_compile()
[open/src/hotspot/share/ci/ciReplay.cpp]
comp_level = TieredCompilation ? TieredStopAtLevel : CompLevel_highest_tier;
Following type changes tried did not help -
- int comp_level = parse_int(comp_level_label);
+ CompLevel comp_level = parse_int(comp_level_label);
.....
- comp_level = TieredCompilation ? TieredStopAtLevel :
CompLevel_highest_tier;
+ comp_level = TieredCompilation ? (CompLevel) TieredStopAtLevel :
CompLevel_highest_tier;
The warning is with only ternary operator usage in this location.
So tried simple code refactoring like following and got no more warnings!
Is this okay?
- comp_level = TieredCompilation ? TieredStopAtLevel :
CompLevel_highest_tier;
+ if (TieredCompilation) {
+ comp_level = TieredStopAtLevel;
+ } else {
+ comp_level = CompLevel_highest_tier;
+ }
3) Op_RegFlags
Warnings only for 'virtual uint MachNode::ideal_reg() const'
../open/src/hotspot/share/opto/machnode.hpp: In member function 'virtual
uint MachNode::ideal_reg() const':
../open/src/hotspot/share/opto/machnode.hpp:304:95: warning: enumeral
and non-enumeral type in conditional expression [-Wextra]
virtual uint ideal_reg() const { const Type *t = _opnds[0]->type();
return t == TypeInt::CC ? Op_RegFlags : t->ideal_reg(); }
~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Op_RegFlags is returned as uint itself here.
How to modify code to solve warning?
Again since the issue is with only ternary operator usage in only one
location, can we go for simple code refactoring like following?
- virtual uint ideal_reg() const { const Type *t = _opnds[0]->type();
return t == TypeInt::CC ? Op_RegFlags : t->ideal_reg(); }
+ virtual uint ideal_reg() const {
+ const Type *t = _opnds[0]->type();
+ if (t == TypeInt::CC) {
+ return Op_RegFlags;
+ } else {
+ return t->ideal_reg();
+ }
+ }
4) _lh_array_tag_obj_value, _lh_instance_slow_path_bit -
warnings locations -
(i) ../open/src/hotspot/share/oops/klass.cpp: In static member function
'static jint Klass::array_layout_helper(BasicType)':
../open/src/hotspot/share/oops/klass.cpp:212:23: warning: enumeral and
non-enumeral type in conditional expression [-Wextra]
int tag = isobj ? _lh_array_tag_obj_value :
_lh_array_tag_type_value;
(ii) ../open/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp: In static member
function 'static OopMapSet*
Runtime1::generate_code_for(Runtime1::StubID, StubAssembler*)':
../open/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp:1126:22: warning:
enumeral and non-enumeral type in conditional expression [-Wextra]
int tag = ((id == new_type_array_id)
~~~~~~~~~~~~~~~~~~~~~~~~~
? Klass::_lh_array_tag_type_value
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
: Klass::_lh_array_tag_obj_value);
(iii) ../open/src/hotspot/share/oops/klass.hpp: In static member
function 'static jint Klass::instance_layout_helper(jint, bool)':
../open/src/hotspot/share/oops/klass.hpp:422:28: warning: enumeral and
non-enumeral type in conditional expression [-Wextra]
| (slow_path_flag ? _lh_instance_slow_path_bit : 0);
~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Following changes fixed the warnings!
(using static const int instead of unnamed enum)
[open/src/hotspot/share/oops/klass.cpp]
..........
// Unpacking layout_helper:
- enum {
- _lh_neutral_value = 0, // neutral non-array non-instance
value
- _lh_instance_slow_path_bit = 0x01,
- _lh_log2_element_size_shift = BitsPerByte*0,
- _lh_log2_element_size_mask = BitsPerLong-1,
- _lh_element_type_shift = BitsPerByte*1,
- _lh_element_type_mask = right_n_bits(BitsPerByte), //
shifted mask
- _lh_header_size_shift = BitsPerByte*2,
- _lh_header_size_mask = right_n_bits(BitsPerByte), //
shifted mask
- _lh_array_tag_bits = 2,
- _lh_array_tag_shift = BitsPerInt - _lh_array_tag_bits,
- _lh_array_tag_obj_value = ~0x01 // 0x80000000 >> 30
- };
+ static const int _lh_neutral_value = 0; // neutral
non-array non-instance value
+ static const int _lh_instance_slow_path_bit = 0x01;
+ static const int _lh_log2_element_size_shift = BitsPerByte*0;
+ static const int _lh_log2_element_size_mask = BitsPerLong-1;
+ static const int _lh_element_type_shift = BitsPerByte*1;
+ static const int _lh_element_type_mask =
right_n_bits(BitsPerByte); // shifted mask
+ static const int _lh_header_size_shift = BitsPerByte*2;
+ static const int _lh_header_size_mask =
right_n_bits(BitsPerByte); // shifted mask
+ static const int _lh_array_tag_bits = 2;
+ static const int _lh_array_tag_shift = BitsPerInt -
_lh_array_tag_bits;
+ static const int _lh_array_tag_obj_value = ~0x01; // 0x80000000
>> 30
.......
<weberev.00> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.00/
Understood the affected code locations details from the old sample patch
attachment of related JDK-8211073
#
https://bugs.openjdk.java.net/secure/attachment/79387/hotspot-disable-wextra.diff
Also confirmed no similar warnings in hotspot/compiler with -Wextra,
no issues with build with this proposed webrev.00
Thanks,
Rahul
More information about the hotspot-compiler-dev
mailing list