Hi Martin, on 2021/9/17 下午7:26, Martin Jambor wrote: > Hi, > > On Fri, Sep 17 2021, Kewen.Lin wrote: >> on 2021/9/16 下午9:19, Martin Jambor wrote: >>> On Thu, Sep 16 2021, Kewen.Lin wrote: >>>> on 2021/9/15 下午8:51, Martin Jambor wrote: >>>>> On Wed, Sep 08 2021, Kewen.Lin wrote: >>>>>> >>>>> >>>>> [...] >>>>> >>>>>> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h >>>>>> index 78399b0b9bb..300b8da4507 100644 >>>>>> --- a/gcc/ipa-fnsummary.h >>>>>> +++ b/gcc/ipa-fnsummary.h >>>>>> @@ -193,6 +194,9 @@ public: >>>>>> vec<ipa_freqcounting_predicate, va_gc> *loop_strides; >>>>>> /* Parameters tested by builtin_constant_p. */ >>>>>> vec<int, va_heap, vl_ptr> GTY((skip)) builtin_constant_p_parms; >>>>>> + /* Like fp_expressions, but it's to hold some target specific >>>>>> information, >>>>>> + such as some target specific isa flags. */ >>>>>> + auto_vec<HOST_WIDE_INT> GTY((skip)) target_info; >>>>>> /* Estimated growth for inlining all copies of the function before >>>>>> start >>>>>> of small functions inlining. >>>>>> This value will get out of date as the callers are duplicated, but >>>>> >>>>> Segher already wrote in the first thread that a vector of HOST_WIDE_INTs >>>>> is an overkill and I agree. So at least make the new field just a >>>>> HOST_WIDE_INT or better yet, an unsigned int. But I would even go >>>>> further and make target_info only a 16-bit bit-field, place it after the >>>>> other bit-fields in class ipa_fn_summary and pass it to the hooks as >>>>> uint16_t. Unless you have plans which require more space, I think we >>>>> should be conservative here. >>>>> >>>> >>>> OK, yeah, the consideration is mainly for the scenario that target has >>>> a few bits to care about. I just realized that to avoid inefficient >>>> bitwise operation for mapping target info bits to isa_flag bits, target >>>> can rearrange the sparse bits in isa_flag, so it's not a deal. >>>> Thanks for re-raising this! I'll use the 16 bits bit-field in v3 as you >>>> suggested, if you don't mind, I will put it before the existing bit-fields >>>> to have a good alignment. >>> >>> All right. >>> >> >> Sorry that I failed to use 16 bit-fields for this, I figured out that >> the bit-fields can not be address-taken or passed as non-const reference. >> The gentype also failed to recognize uint16_t if I used uint16_t directly >> in ipa-fnsummary.h. Finally I used unsigned int instead. >> > > well, you could have used: > > unsigned int target_info : 16; > > for the field (and uint16_t when passed to hooks). > > But I am not sure if it is that crucial. >
I may miss something, specifically I tried with: 1) unsigned int target_info : 16; unsigned inlinable : 1; ... update_ipa_fn_target_info (uint16_t &, const gimple *) 2) unsigned int target_info : 16; unsigned inlinable : 1; ... update_ipa_fn_target_info (uint16_t *, const gimple *) The above two ways failed due to: "Because bit fields do not necessarily begin at the beginning of a byte, address of a bit field cannot be taken. Pointers and non-const references to bit fields are not possible." as [1]. Although we can change the hook prototype to bool update_ipa_fn_target_info (const uint16_t, const gimple*, uint16_t&) or uint16_t update_ipa_fn_target_info (const uint16_t, const gimple*, bool&) to workaround bit field limitation, it looks weird and inefficient. 3) ... unsigned int fp_expressions : 1; uint16_t target_info; update_ipa_fn_target_info (uint16_t &, const gimple *) it fails due to gengtype erroring: gcc/ipa-fnsummary.h:171: undefined type `uint16_t' gengtype: didn't write state file tmp-gtype.state after errors Then I gave up and guessed it's not so crucial like you said, and used unsigned int instead. :) [1] https://en.cppreference.com/w/cpp/language/bit_field BR, Kewen
