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.
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.
>
> 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). :)
BR,
Kewen
