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
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.
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?
+ built_in_class klass = BUILT_IN_NORMAL)
This deviates from the gimple.h routines as well (also builtin vs. built_in, but
built_in is better).
+{
+ 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(-)
> >>>>
> >>>>
> >>>
> >>
>
