Hi Martin,
Thanks for the review.
on 2021/9/18 下午7:31, Martin Jambor wrote:
> Hi,
>
> On Fri, Sep 17 2021, Segher Boessenkool wrote:
>> On Fri, Sep 17, 2021 at 05:42:38PM +0800, Kewen.Lin wrote:
>>> Against v2 [2], this v3 addressed Martin's review comments:
>>> - Replace HWI auto_vec with unsigned int for target_info
>>> to avoid overkill (also Segher's comments), adjust some
>>> places need to be updated for this change.
>>
>> I'd have used a single HWI (always 64 bits), but an int (always at least
>> 32 bits in GCC) is fine as well, sure.
>
> Let's have it as unsigned int then (in a separate thread I was
> suggesting to go even smaller).
>
>> That easily fits one line. (Many more examples here btw).
>>
>>> +bool
>>> +rs6000_fn_has_any_of_these_mask_bits (enum rs6000_builtins code,
>>> + HOST_WIDE_INT mask)
>>> +{
>>> + gcc_assert (code < RS6000_BUILTIN_COUNT);
>>
>> We don't have this assert anywhere else, so lose it here as well?
>>
>> If we want such checking we should make an inline accessor function for
>> this, and check it there. But we already do a check in
>> rs6000_builtin_decl (and one in def_builtin, but that one has an
>> off-by-one error in it).
>>
>>> +extern bool rs6000_fn_has_any_of_these_mask_bits (enum rs6000_builtins
>>> code,
>>> + HOST_WIDE_INT mask);
>>
>> The huge unwieldy name suggests it might not be the best abstraction you
>> could use, btw ;-)
>>
>>> +static bool
>>> +rs6000_update_ipa_fn_target_info (unsigned int &info, const gimple *stmt)
>>> +{
>>> + /* Assume inline asm can use any instruction features. */
>>> + if (gimple_code (stmt) == GIMPLE_ASM)
>>
>> This should be fine for HTM, but it may be a bit *too* pessimistic for
>> other features. We'll see when we get there :-)
>>
>>> +@deftypefn {Target Hook} bool TARGET_NEED_IPA_FN_TARGET_INFO (const_tree
>>> @var{decl}, unsigned int& @var{info})
>>> +Allow target to check early whether it is necessary to analyze all gimple
>>> +statements in the given function to update target specific information for
>>> +inlining. See hook @code{update_ipa_fn_target_info} for usage example of
>> [ ... ]
>>> +The default version of this hook returns false.
>>
>> And that is really the only reason to have this premature optimisation:
>> targets that do not care do not have to pay the price, however trivial
>> that price may be, which is a good idea politically ;-)
>>
>>> +/* { dg-final { scan-tree-dump-times "Inlining foo/\[0-9\]* " 1 "einline"}
>>> } */
>>
>> If you use {} instead of "" you don't need the backslashes.
>>
>>> +default_update_ipa_fn_target_info (uint16_t &, const gimple *)
>>
>> I'm surprised the compiler didn't warn about this btw.
>>
>> The rs6000 parts are okay for trunk (with the trivial cleanups please).
>> Thanks!
>
>> +/* By default, return false to not need to collect any target information
>> + for inlining. Target maintainer should re-define the hook if the
>> + target want to take advantage of it. */
>> +
>> +bool
>> +default_need_ipa_fn_target_info (const_tree, uint16_t &)
>> +{
>> + return false;
>> +}
>> +
>> +bool
>> +default_update_ipa_fn_target_info (uint16_t &, const gimple *)
>> +{
>> + return false;
>> +}
>
> The parameters have uint16_t type here but you apparently decided to use
unsigned int everywhere else, you probably forgot to change them here
too.
Yeah, I did forget to change them. :( Thanks for catching!
> the IPA bits are OK too (after the type mismatch is fixed).
>
Thanks!
BR,
Kewen
