Hi Bill!
on 2021/10/13 上午12:36, Bill Schmidt wrote:
> 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.
>
Thanks for the explanation, it makes more sense.
>>
>> 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!
>
Thanks for confirming!
Segher, we think this patch is safe and it also makes the function
behave more consistently as the description of the hook.
Does this patch look good to you?
>>
>>> 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.
Got it, thanks!
BR,
Kewen
