[PATCH] clarify comments for implicit_p flag for built-ins

[Richard Biener]

On Wed, Nov 28, 2018 at 7:18 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 11/28/18 6:35 AM, Richard Biener wrote:
> > On Tue, Nov 27, 2018 at 3:52 AM Martin Sebor <msebor@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
> >>
>