[13] RFR: 8213416: Replace some enums with static const members in hotspot/compiler
David Holmes
david.holmes at oracle.com
Thu May 16 05:43:42 UTC 2019
This all seems like unnecessary churn to me - is any of this code
actually wrong? can we not just disable this particular warning? is
there any point using "static const" when we should be aiming to use
C++11 constexpr in the (not too distant?) future?
Converting from enums to unrelated ints seems a big step backwards in
software engineering terms. :(
Cheers,
David
-----
On 16/05/2019 12:46 am, Rahul Raghavan wrote:
> 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