Hi Kewen,
On 10/11/21 1:30 AM, Kewen.Lin wrote:
> Hi Segher,
>
> Thanks for the comments.
>
> on 2021/10/1 上午6:13, Segher Boessenkool wrote:
>> Hi!
>>
>> On Thu, Sep 30, 2021 at 11:06:50AM +0800, Kewen.Lin wrote:
>>
>> [ huge snip ]
>>
>>> Based on the understanding and testing, I think it's safe to adopt this
>>> patch.
>>> Do both Peter and you agree the rs6000_expand_builtin will catch the
>>> invalid built-in?
>>> Is there some special case which probably escapes out?
>> The function rs6000_builtin_decl has a terribly generic name. Where all
>> is it called from? Do all such places allow the change in semantics?
>> Do any comments or other documentation need to change? Is the function
>> name still good?
>
> % grep -rE "\<builtin_decl\> \(" .
> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true);
> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true);
> ./gcc/config/avr/avr-c.c: fold = targetm.builtin_decl (id, true);
> ./gcc/config/aarch64/aarch64.c: return aarch64_sve::builtin_decl
> (subcode, initialize_p);
> ./gcc/config/aarch64/aarch64-protos.h: tree builtin_decl (unsigned, bool);
> ./gcc/config/aarch64/aarch64-sve-builtins.cc:builtin_decl (unsigned int code,
> bool)
> ./gcc/tree-streamer-in.c: tree result = targetm.builtin_decl (fcode,
> true);
>
> % grep -rE "\<rs6000_builtin_decl\> \(" .
> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl
> (instance->bifid, false) != error_mark_node
> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl
> (instance->bifid, false) != error_mark_node
> ./gcc/config/rs6000/rs6000-c.c: if (rs6000_builtin_decl
> (instance->bifid, false) != error_mark_node
> ./gcc/config/rs6000/rs6000-gen-builtins.c: "extern tree
> rs6000_builtin_decl (unsigned, "
> ./gcc/config/rs6000/rs6000-call.c:rs6000_builtin_decl (unsigned code, bool
> initialize_p ATTRIBUTE_UNUSED)
> ./gcc/config/rs6000/rs6000-internal.h:extern tree rs6000_builtin_decl
> (unsigned code,
>
> As above, the call sites are mainly in
> 1) function unpack_ts_function_decl_value_fields in gcc/tree-streamer-in.c
> 2) function altivec_resolve_new_overloaded_builtin in
> gcc/config/rs6000/rs6000-c.c
>
> 2) is newly introduced by Bill's bif rewriting patch series, all uses in it
> are
> along with rs6000_new_builtin_is_supported which adopts a new way to check bif
> supported or not (the old rs6000_builtin_is_supported_p uses builtin mask), so
> I think the builtin mask checking is useless (unexpected?) for these uses.
Things are a bit confused because we are part way through the patch series.
rs6000_builtin_decl will be changed to redirect to rs6000_new_builtin_decl when
using the new builtin support. That function will be:
static tree
rs6000_new_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED)
{
rs6000_gen_builtins fcode = (rs6000_gen_builtins) code;
if (fcode >= RS6000_OVLD_MAX)
return error_mark_node;
if (!rs6000_new_builtin_is_supported (fcode))
{
rs6000_invalid_new_builtin (fcode);
return error_mark_node;
}
return rs6000_builtin_decls_x[code];
}
So, as you surmise, this will be using the new method of testing for builtin
validity.
You can ignore the rs6000-c.c and rs6000-gen-builtins.c references of
rs6000_builtin_decl
for purposes of fixing the existing way of doing things.
>
> Besides, the description for this hook:
>
> "tree TARGET_BUILTIN_DECL (unsigned code, bool initialize_p) [Target Hook]
> Define this hook if you have any machine-specific built-in functions that
> need to be
> defined. It should be a function that returns the builtin function
> declaration for the
> builtin function code code. If there is no such builtin and it cannot be
> initialized at
> this time if initialize p is true the function should return NULL_TREE. If
> code is out
> of range the function should return error_mark_node."
>
> It would only return error_mark_node when the code is out of range. The
> current
> rs6000_builtin_decl returns error_mark_node not only for "out of range", it
> looks
> inconsistent and this patch also revise it.
>
> The hook was introduced by commit e9e4b3a892d0d19418f23bb17bdeac33f9a8bfd2,
> it meant to ensure the bif function_decl is valid (check if bif code in the
> range and the corresponding entry in bif table is not NULL). May be better
> with name check_and_get_builtin_decl? CC Richi, he may have more insights.
>
>>> By the way, I tested the bif rewriting patch series V5, it couldn't make
>>> the original
>>> case in PR (S5) pass, I may miss something or the used series isn't
>>> up-to-date. Could
>>> you help to have a try? I agree with Peter, if the rewriting can fix this
>>> issue, then
>>> we don't need this patch for trunk any more, I'm happy to abandon this. :)
>> (Mail lines are 70 or so chars max, so that they can be quoted a few
>> levels).
>>
> ah, OK, thanks. :)
>
>> If we do need a band-aid for 10 and 11 (and we do as far as I can see),
>> I'd like to see one for just MMA there, and let all other badness fade
>> into history. Unless you can convince me (in the patch / commit
>> message) that this is safe :-)
> Just to fix for MMA seems incomplete to me since we can simply
> construct one non-MMA but failed case. I questioned in the other
> thread, is there any possibility for one invalid target specific
> bif to escape from the function rs6000_expand_builtin? (note that
> folding won't handle invalid bifs, so invalid bifs won't get folded
> early.) If no, I think it would be safe.
Yes, looking at what you wrote in the other thread about your investigations
with -DEXPECT_ERROR, the existing rs6000_expand_builtin logic makes the
patch safe from my perspective. Thank you for digging in and checking!
>
>> Whichever way you choose, it is likely best to do the same on 10 and 11
>> as on trunk, since it will all be replaced on trunk soon anyway.
>>
> OK, will see Bill's reply (he should be back from vacation soon). :)
I am back! :) Agree with Segher that backporting this after a sensible
interval would be appropriate. We have been fixing these things
piecemeal for too long, so if we have a solid general fix, I'm a fan.
Going forward with 12 and later we can rip off the band-aid and do things
right.
Thanks!
Bill
>
> BR,
> Kewen
