On Wed, Nov 28, 2018 at 7:18 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 11/28/18 6:35 AM, Richard Biener wrote:
> > On Tue, Nov 27, 2018 at 3:52 AM Martin Sebor <mse...@gmail.com> wrote:
> >>
> >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01759.html
> >>
> >> If there are no objections or suggestions for tweaks I'll commit
> >> this updated comment this week.
> >
> > Please do not commit such changes w/o approval.
>
> Let me follow up on this separately.
>
>
> > Does the new comments match existing usage, for example in gimplify.c
> > where we do
> >
> > static void
> > build_stack_save_restore (gcall **save, gcall **restore)
> > {
> > tree tmp_var;
> >
> > *save = gimple_build_call (builtin_decl_implicit (BUILT_IN_STACK_SAVE),
> > 0);
> > tmp_var = create_tmp_var (ptr_type_node, "saved_stack");
> > gimple_call_set_lhs (*save, tmp_var);
> >
> > *restore
> > = gimple_build_call (builtin_decl_implicit (BUILT_IN_STACK_RESTORE),
> > 1, tmp_var);
> > }
> >
> > ?
>
> I don't know why this code uses builtin_decl_implicit here. I don't
> think it's necessary or even correct. If builtin_decl_implicit were
> to return null gimple_build_call() would ICE. Replacing it with
> builtin_decl_explicit passes an x86_64 bootstrap and regression test.
>
> I see the same pattern in some of the other calls to
> builtin_decl_implicit with non-library built-ins. E.g., in
> gimplify_function_tree:
>
> x = builtin_decl_implicit (BUILT_IN_RETURN_ADDRESS);
> call = gimple_build_call (x, 1, integer_zero_node);
>
> > +/* Return the tree node for the built-in function declaration corresponding
> > + to FNCODE if its IMPLICIT_P flag has been set or NULL otherwise.
> > + IMPLICIT_P is clear for library built-ins that GCC implements but that
> > + may not be implemented in the runtime library on the target. See also
> > + the DEF_BUILTIN macro in builtins.def. */
> > +
> >
> > it isn't exactly helpful to specify when IMPLICIT_P is not set. When is it
> > actually set?
> >
> > My understanding of the difference is that the compiler may create a call
> > to a builtin_decl_implicit decl but _not_ to a builtin_decl_explicit one
> > unless a call to such function already existed.
>
> This sounds like an important detail to capture but I'm not sure
> I quite understand it to do so.
>
> Besides the BUILT_IN_STACK question above, cp/cp-gimplify.c for
> example calls builtin_decl_explicit with BUILT_IN_UNREACHABLE
> to unconditionally insert a call to the built-in. It does that
> whether or not a call to the built-in has been seen in the program.
> E.g., this:
>
> int f () { }
> int g () { return f (); }
>
> ends up with a __builtin_unreachable call (IMPLICIT is set here
> so the result would be the same if cp/cp-gimplify.c called
> builtin_decl_implicit instead, but that seems beside the point).
>
> > Note we set the implicit flag during gimplification if we see such decl
> > referenced and it is marked as properly declared by the FE.
>
> I see this in gimplify.c:
>
> /* If we see a call to a declared builtin or see its address
> being taken (we can unify those cases here) then we can mark
> the builtin for implicit generation by GCC. */
> if (TREE_CODE (op0) == FUNCTION_DECL
> && fndecl_built_in_p (op0, BUILT_IN_NORMAL)
> && builtin_decl_declared_p (DECL_FUNCTION_CODE (op0)))
> set_builtin_decl_implicit_p (DECL_FUNCTION_CODE (op0), true);
>
> This makes sense to me for library built-ins but for others like
> __builtin_unreachable it seems unnecessary. Their IMPLICIT flag
> is already set. The only calls to set_builtin_decl_implicit_p
> that I can find that clear the IMPLICIT flag are in the VMS
> back-end for BUILT_IN_FWRITE (apparently because the function
> is non-standard there).
>
> >
> > Overall your patch is an improvement I think but maybe you can
> > refer to builtins.def in the documentation for builtin_decl_implicit?
>
> Sure, I'm happy to adjust it once we're confident our understanding
> of how the flag works is correct. In light of your comments I for
> one don't feel I'm there yet.
Everytime I get to the difference of implicit vs. explicit I am confused
as well. Thus I am not saying I completely capture the difference...
(given all the inconsistencies in uses throughout the GCC codebase).
In fact I fail to see the need to have both - having only _implicit would
seem enough for the middle-end ...
Richard.
> Thanks
> Martin
>
> >
> >> On 11/20/18 1:24 PM, Martin Sebor wrote:
> >>> On 11/20/2018 11:02 AM, Martin Sebor wrote:
> >>>> Would the updated comments in the attached patch more accurately
> >>>> describe the purpose of the IMPLICIT_P flag and
> >>>> the builtin_decl_explicit() and builtin_decl_implicit() functions?
> >>>>
> >>>> I ended up here while trying to understand the differences between
> >>>> the functions on different targets and decide which one should be
> >>>> used to diagnose bugs like the one below:
> >>>>
> >>>> long double fabsl (); // missing prototype
> >>>>
> >>>> long double f (int x)
> >>>> {
> >>>> return fabsl (x); // want a warning here
> >>>> }
> >>>>
> >>>> I think we want the warning regardless of IMPLICIT_P so the warning
> >>>> code should call builtin_decl_explicit() to obtain fabsl's expected
> >>>> type, even if the target's runtime doesn't support the function on
> >>>> the basis of the comment:
> >>>>
> >>>> When a program uses floorf we may assume that the floorf function
> >>>> has the expected meaning
> >>>
> >>> Actually, some more testing suggests the comment in builtins.def
> >>> (either the original or the patched one) isn't entirely accurate
> >>> or helpful to understanding the purpose of the flag:
> >>>
> >>> IMPLICIT specifies condition when the builtin can be produced by
> >>> compiler. For instance C90 reserves floorf function, but does not
> >>> define it's meaning. When user uses floorf we may assume that the
> >>> floorf has the meaning we expect, but we can't produce floorf by
> >>> simplifying floor((double)float) since the runtime need not
> >>> implement it.
> >>>
> >>> Given the following code:
> >>>
> >>> float floorf ();
> >>>
> >>> void f (void)
> >>> {
> >>> if (floorf (0.0f))
> >>> __builtin_abort ();
> >>> }
> >>>
> >>> in C90 mode, BUILT_IN_FLOORF's IMPLICIT flag is clear and GCC
> >>> doesn't seem to assume anything about the call to the function,
> >>> contrary to the comment ("we may assume the meaning we expect").
> >>> The comment also doesn't explain when IMPLICIT may be set.
> >>>
> >>> I've updated the comment a bit more to more accurately describe
> >>> when I think the flag is set or clear, and how it's used.
> >>> Corrections or further clarification are appreciated.
> >>>
> >>> Thanks
> >>> Martin
> >>
>
