[13] RFR: 8213416: Replace some enums with static const members in hotspot/compiler
Rahul Raghavan
rahul.v.raghavan at oracle.com
Fri May 24 20:17:08 UTC 2019
Hi,
Request one more review approval for latest <webrev.01>
http://cr.openjdk.java.net/~rraghavan/8213416/webrev.01/
Thanks,
Rahul
On 20/05/19 11:46 PM, Rahul Raghavan wrote:
> Hi,
>
> With reference to below email thread, request help to confirm next steps
> for JDK-8213416.
>
> So may I go ahead with webrev changes related to hotspot/compiler for
> 8213416 ?
> <webrev.01> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.01/
>
> (also will add similar hotspot/runtime related details in JBS comments
> for JDK-8223400)
>
> Thanks,
> Rahul
>
>
>
> On 16/05/19 3:26 PM, Rahul Raghavan wrote:> Hi,
> >
> > Thank you David for review comments.
> > I will kindly request help from Magnus to reply for the main questions.
> >
> > Sharing some notes, related links -
> > - 8211073: Remove -Wno-extra from Hotspot
> > https://bugs.openjdk.java.net/browse/JDK-8211073
> > - Discussions in earlier thread -
> >
> https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-September/034314.html
> >
> > So understood -Wextra do help in catching valid/useful warnings also,
> > but along with some too strict ones like "enumeral and non-enumeral type
> > in conditional expression" type warnings.
> >
> > Extracts from 8211073 JBS comments from Magnus regarding the
> > 'enum-warning' -
> > "... If you think that gcc is a bit too picky here, I agree. It's not
> > obvious per se that the added casts improve the code. However, this is
> > the price we need to pay to be able to enable -Wextra, and *that* is
> > something that is likely to improve the code."
> >
> >
> > Thanks,
> > Rahul
> >
> > On 16/05/19 11:13 AM, David Holmes wrote:
> >> 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/19 12:03 PM, Rahul Raghavan wrote:
>> Hi,
>>
>> Thank you Vladimir for review comments.
>>
>>>> 4) _lh_array_tag_obj_value, _lh_instance_slow_path_bit -
>>>> [open/src/hotspot/share/oops/klass.cpp]
>>>> ..........
>>>
>>> I am okay with it but Runtime group should agree too - it is their code.
>>>
>> Yes, I missed that it is Runtime code.
>>
>> Please note plan is to handle only the hotspot/compiler part of the
>> requirement changes in JDK-8213416.
>> As per earlier JBS comments new JDK-8223400 was created to cover the
>> requirements in hotspot/runtime.
>> So may I suggest moving the above runtime change requirement details
>> to JDK-822340;
>> and use only the balance changes, as in below updated webrev, here for
>> 8213416.
>>
>> <webrev.01> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.01/
>>
>>
>> Thanks,
>> Rahul
On 15/05/19 10:01 PM, Vladimir Kozlov wrote:> Hi Rahul,
>
> Comments are inlined below.
>
> On 5/15/19 7: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;
>>
>
> Okay.
>
>>
>>
>> 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;
>>
>
> Thank you for explaining that it did not work.
>>
>> 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 > + }
>>
>
> Good.
>
>>
>>
>> 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();
>> + }
>> + }
>>
>
> Good. I agree.
>
>>
>>
>> 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
>> .......
>>
>
> I am okay with it but Runtime group should agree too - it is their code.
>
>>
>>
>> <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
>
> Good.
>
> Thanks,
> Vladimir
>
>>
>>
>> Thanks,
>> Rahul
More information about the hotspot-compiler-dev
mailing list