On Mon, Aug 27, 2018 at 9:20 AM Martin Liška <mli...@suse.cz> wrote:
>
> On 08/23/2018 03:58 PM, Richard Biener wrote:
> > On Thu, Aug 23, 2018 at 3:30 PM Martin Liška <mli...@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 <mli...@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 <mli...@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.
>
> Hello.
>
> I reworked the patch that way.
>
> >
> >>>
> >>> 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?
>
> Yes, good idea.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
You left the NULL checks in the code but still have
+inline bool
+fndecl_built_in_p (const_tree node)
+{
+ return (node != NULL_TREE
+ && DECL_BUILT_IN_CLASS (node) != NOT_BUILT_IN);
+}
Please remove the now redundant NULL check here.
OK with that change,
Richard.
> Martin
>
> >
> >>> 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 <mli...@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 <mli...@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 <mli...@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(-)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
