This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Use more DECL_BUILT_IN_P macro.


On Thu, Aug 23, 2018 at 3:30 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 08/23/2018 01:58 PM, Richard Biener wrote:
> > On Thu, Aug 23, 2018 at 12:46 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 08/20/2018 10:34 AM, Richard Biener wrote:
> >>> On Wed, Aug 15, 2018 at 2:52 PM Martin Liška <mliska@suse.cz> wrote:
> >>>>
> >>>> On 08/14/2018 06:02 PM, Martin Sebor wrote:
> >>>>> On 08/14/2018 03:06 AM, Martin Liška wrote:
> >>>>>> Hi.
> >>>>>>
> >>>>>> The patch adds more usages of the new macro. I hope it improves
> >>>>>> readability of code.
> >>>>>
> >>>>> I think it does :)  I see that most invocations of it in your
> >>>>> patch are with BUILT_IN_NORMAL as the second argument.  Is
> >>>>> the argument implied by the last argument?  E.g., in
> >>>>>
> >>>>>   DECL_BUILT_IN_P (fndecl, BUILT_IN_NORMAL, BUILT_IN_VA_ARG_PACK)
> >>>>>
> >>>>> is there a way to determine that BUILT_IN_VA_ARG_PACK implies
> >>>>> DECL_BUILT_IN_CLASS(fndecl), so that the second argument could
> >>>>> be eliminated?  (Both to make the invocation less verbose and
> >>>>> to avoid the mistake of using the macro with the wrong class.)
> >>>>
> >>>> If I see correctly not, there are separate enums:
> >>>>
> >>>> BUILT_IN_MD:
> >>>>
> >>>> enum ix86_builtins {
> >>>>   IX86_BUILTIN_MASKMOVQ,
> >>>>   IX86_BUILTIN_LDMXCSR,
> >>>>   IX86_BUILTIN_STMXCSR,
> >>>> ...
> >>>> }
> >>>>
> >>>> BUILT_IN_NORMAL:
> >>>> enum built_in_function {
> >>>> BUILT_IN_NONE,
> >>>> BUILT_IN_ACOS,
> >>>> BUILT_IN_ACOSF,
> >>>> ...
> >>>> }
> >>>>
> >>>> So the enum values overlap and thus one needs the class.
> >>>>
> >>>>
> >>>>>
> >>>>> If not, adding DECL_NORMAL_BUILT_IN_P() would make it possible
> >>>>> to omit it.
> >>>>
> >>>> But yes, this is nice idea, I'm sending V2 of the patch that I'm testing
> >>>> right now.
> >>>
> >>> Ick, and now we have DECL_IS_BUILTIN, DECL_BUILT_IN, DECL_BUILT_IN_P
> >>> and DECL_NORMAL_BUILT_IN_P ... (esp the first one is confusing).
> >>
> >> Agree.
> >>
> >>>
> >>> I think following what gimple.h does would be nicer which means using
> >>> inline functions and overloading.
> >>>
> >>> decl_built_in_p (tree);
> >>> decl_built_in_p (tree, enum built_in_class);
> >>> decl_built_in_p (tree, enum built_in_function); /// implies BUILT_IN_NORMAL
> >>
> >> Yes, that's easier to use!
> >>
> >>>
> >>> Can you rework things this way please?  (ok, for gimple those are not inlines)
> >>
> >> Done in patch that can bootstrap and survives regression tests.
> >> Ready for trunk?
> >
> > +/* For a FUNCTION_DECL NODE, nonzero means a built in function of a
> > +   standard library or more generally a built in function that is
> > +   recognized by optimizers and expanders.
> > +
> > +   Note that it is different from the DECL_IS_BUILTIN accessor.  For
> > +   instance, user declared prototypes of C library functions are not
> > +   DECL_IS_BUILTIN but may be DECL_BUILT_IN.  */
> > +
> > +inline bool
> > +decl_built_in_p (const_tree node)
> > +{
> > +  return (node != NULL_TREE
> > +         && TREE_CODE (node) == FUNCTION_DECL
> >
> > You document for a FUNCTION_DECL NODE but the check
> > for FUNCTION_DECL anyway.  I realize doing all (possibly redundant)
> > checks in decl_built_in_p is convenient at callers, but at least update
> > docs accordingly?  In reality I somewhat prefer
>
> Yes, please let stay with version that does the checks. It can provide
> more readable code like:
>
>        /* If last argument is __builtin_va_arg_pack (), arguments to this
>          function are not finalized yet.  Defer folding until they are.  */
>        if (n && TREE_CODE (argarray[n - 1]) == CALL_EXPR)
>         {
>           tree fndecl2 = get_callee_fndecl (argarray[n - 1]);
> -         if (fndecl2
> -             && TREE_CODE (fndecl2) == FUNCTION_DECL
> -             && DECL_BUILT_IN_CLASS (fndecl2) == BUILT_IN_NORMAL
> -             && DECL_FUNCTION_CODE (fndecl2) == BUILT_IN_VA_ARG_PACK)
> +         if (decl_built_in_p (fndecl2, BUILT_IN_VA_ARG_PACK))
>             return NULL_TREE;
>         }
>        if (avoid_folding_inline_builtin (fndecl))

But here the TREE_CODE (fndecl2) == FUNCTION_DECL check is redundant
(get_callee_fndecl always returns a FUNCTION_DECL).  So it would become

  if (fndecl2 && fndecl_built_in_p (fndecl2, BUILT_IN_VA_ARG_PACK))

which is IMHO good enough.

> >
> > inline bool
> > decl_built_in_p (const_tree node)
> > {
> >   return DECL_BUILT_IN_CLASS (node) != NOT_BUILT_IN;
> > }
> >
> > and
> >
> > inline bool
> > decl_built_in_p (const_tree node, built_in_class klass)
> > {
> >   return DECL_BUILT_IN_CLASS (node) == klass;
> > }
> >
> > esp. since the built_in_class overload doesn't work for
> > NOT_BUILT_IN for your variant.
>
> That's true. One should probably use DECL_BUILT_IN_CLASS (t1) != NOT_BUILT_IN
> in this case.
>
> >
> > And if we're at changing, maybe call the functions
> > fndecl_built_in_p to make it clear we're expecting FUNCTION_DECLs...
> >
> > +/* For a FUNCTION_DECL NODE, return true when a function is
> > +   a built-in of class KLASS with name equal to NAME.  */
> > +
> > +inline bool
> > +decl_built_in_p (const_tree node, int name,
> >
> > why's that 'int' and not built_in_function?
>
> Because we want to also call it for e.g. cp_built_in_function
> enum types:
>
> gcc/cp/cp-gimplify.c:2498:26: error: no matching function for call to ‘decl_built_in_p(tree_node*&, cp_built_in_function, built_in_class)’
>
> Note that:
> gimple_call_builtin_p (const gimple *stmt, enum built_in_function code)
>
> is only used for BUILT_IN_NORMAL, which decl_built_in_p should handle also FE and TARGET codes.

I know ...

> >
> > +                built_in_class klass = BUILT_IN_NORMAL)

So without the default arg we could make two overloads:

fndecl_built_in_p (const_tree, enum built_in_function)
fndecl_built_in_p (const_tree, int, enum built_in_class)

and keep the type checking?

> > This deviates from the gimple.h routines as well (also builtin vs. built_in, but
> > built_in is better).
>
> As you with here, I'm fine with both. Are you fine with the consensus that the functions will
> do check for null argument and TREE_CODE check?

Not yet ;)  The extra checks are not going to be optimized away in
those places that do
not need them I think.  Any more convincing examples?

Richard.

> Martin
>
> >
> > +{
> > +  return (decl_built_in_p (node, klass) && DECL_FUNCTION_CODE (node) == name);
> > +}
> >
> >
> >> Martin
> >>
> >>>
> >>> Thanks,
> >>> Richard.
> >>>
> >>>> Martin
> >>>>
> >>>>>
> >>>>> Martin
> >>>>>
> >>>>>>
> >>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> >>>>>>
> >>>>>> Ready to be installed?
> >>>>>> Martin
> >>>>>>
> >>>>>> gcc/ChangeLog:
> >>>>>>
> >>>>>> 2018-08-10  Martin Liska  <mliska@suse.cz>
> >>>>>>
> >>>>>>     * tree.h (DECL_BUILT_IN_P): Add also check
> >>>>>>         for TREE_CODE (NODE) == FUNCTION_DECL.
> >>>>>>     * builtins.c (fold_call_expr): Use DECL_BUILT_IN_P macro.
> >>>>>>     (fold_builtin_call_array): Likewise.
> >>>>>>     * cgraph.c (cgraph_update_edges_for_call_stmt_node): Likewise.
> >>>>>>     (cgraph_edge::verify_corresponds_to_fndecl): Likewise.
> >>>>>>     (cgraph_node::verify_node): Likewise.
> >>>>>>     * cgraphclones.c (cgraph_node::create_clone): Likewise.
> >>>>>>     * dse.c (scan_insn): Likewise.
> >>>>>>     * fold-const.c (fold_binary_loc): Likewise.
> >>>>>>     * gimple-pretty-print.c (dump_gimple_call): Likewise.
> >>>>>>     * gimple.c (gimple_call_builtin_p): Likewise.
> >>>>>>     * gimplify.c (gimplify_call_expr): Likewise.
> >>>>>>     (gimple_boolify): Likewise.
> >>>>>>     (gimplify_modify_expr): Likewise.
> >>>>>>     * ipa-fnsummary.c (compute_fn_summary): Likewise.
> >>>>>>     * omp-low.c (setjmp_or_longjmp_p): Likewise.
> >>>>>>     * trans-mem.c (is_tm_irrevocable): Likewise.
> >>>>>>     (is_tm_abort): Likewise.
> >>>>>>     * tree-cfg.c (stmt_can_terminate_bb_p): Likewise.
> >>>>>>     * tree-inline.c (copy_bb): Likewise.
> >>>>>>     * tree-sra.c (scan_function): Likewise.
> >>>>>>     * tree-ssa-ccp.c (optimize_stack_restore): Likewise.
> >>>>>>     (pass_fold_builtins::execute): Likewise.
> >>>>>>     * tree-ssa-dom.c (dom_opt_dom_walker::optimize_stmt): Likewise.
> >>>>>>     * tree-ssa-loop-im.c (stmt_cost): Likewise.
> >>>>>>     * tree-stdarg.c (optimize_va_list_gpr_fpr_size): Likewise.
> >>>>>>
> >>>>>> gcc/c/ChangeLog:
> >>>>>>
> >>>>>> 2018-08-10  Martin Liska  <mliska@suse.cz>
> >>>>>>
> >>>>>>     * c-parser.c (c_parser_postfix_expression_after_primary):
> >>>>>>         Use DECL_BUILT_IN_P macro.
> >>>>>>
> >>>>>> gcc/cp/ChangeLog:
> >>>>>>
> >>>>>> 2018-08-10  Martin Liska  <mliska@suse.cz>
> >>>>>>
> >>>>>>     * constexpr.c (constexpr_fn_retval): Use DECL_BUILT_IN_P macro.
> >>>>>>     (cxx_eval_builtin_function_call): Likewise.
> >>>>>>     * cp-gimplify.c (cp_gimplify_expr): Likewise.
> >>>>>>     (cp_fold): Likewise.
> >>>>>>     * semantics.c (finish_call_expr): Likewise.
> >>>>>>     * tree.c (builtin_valid_in_constant_expr_p): Likewise.
> >>>>>> ---
> >>>>>>  gcc/builtins.c            | 10 ++--------
> >>>>>>  gcc/c/c-parser.c          |  9 +++------
> >>>>>>  gcc/cgraph.c              | 13 ++++++-------
> >>>>>>  gcc/cgraphclones.c        |  4 ++--
> >>>>>>  gcc/cp/constexpr.c        | 12 +++++-------
> >>>>>>  gcc/cp/cp-gimplify.c      | 12 ++++--------
> >>>>>>  gcc/cp/semantics.c        |  4 +---
> >>>>>>  gcc/cp/tree.c             |  5 ++---
> >>>>>>  gcc/dse.c                 |  5 ++---
> >>>>>>  gcc/fold-const.c          |  4 +---
> >>>>>>  gcc/gimple-pretty-print.c |  4 +---
> >>>>>>  gcc/gimple.c              |  3 +--
> >>>>>>  gcc/gimplify.c            | 14 ++++----------
> >>>>>>  gcc/ipa-fnsummary.c       |  8 ++++----
> >>>>>>  gcc/omp-low.c             |  5 ++---
> >>>>>>  gcc/trans-mem.c           |  8 ++------
> >>>>>>  gcc/tree-cfg.c            |  3 +--
> >>>>>>  gcc/tree-inline.c         |  4 ++--
> >>>>>>  gcc/tree-sra.c            |  4 ++--
> >>>>>>  gcc/tree-ssa-ccp.c        |  8 ++------
> >>>>>>  gcc/tree-ssa-dom.c        |  4 +---
> >>>>>>  gcc/tree-ssa-loop-im.c    |  4 +---
> >>>>>>  gcc/tree-stdarg.c         |  6 ++----
> >>>>>>  gcc/tree.h                |  4 +++-
> >>>>>>  24 files changed, 56 insertions(+), 101 deletions(-)
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]