Hi Segher,
Thanks for the review!
on 2021/9/17 下午10:14, 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.
>
>> * config/rs6000/rs6000-internal.h
>> (rs6000_fn_has_any_of_these_mask_bits): New declare.
>
> You can break that after the ":"... Just :-)
>
OK, will adjust.
>> * doc/tm.texi.in (TARGET_UPDATE_IPA_FN_TARGET_INFO): Document new
>> hook.
>
> That easily fits one line. (Many more examples here btw).
>
I noticed some practices seem to like breaking it early, maybe due to
it could have a good overall alignment view? I am guessing the demand
for where to break isn't that strict?
>> +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).
OK, will remove 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.
>
OK, will adjust.
>> +default_update_ipa_fn_target_info (uint16_t &, const gimple *)
>
> I'm surprised the compiler didn't warn about this btw.
>
So am I. Guessing it's due to I only bootstrapped it on ppc64le which
re-defines
the hook? Anyway, I will do the testing on x86 as well before committing it.
> The rs6000 parts are okay for trunk (with the trivial cleanups please).
> Thanks!
>
Thanks again!
BR,
Kewen
