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!
the IPA bits are OK too (after the type mismatch is fixed).
Martin
