RFA: Disentangle builtin folding from expanding
Richard Guenther
richard.guenther@gmail.com
Mon Sep 28 09:28:00 GMT 2009
On Mon, Sep 28, 2009 at 1:50 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 23 Sep 2009, Paolo Bonzini wrote:
>
>> > > This patch as is regstraps on x86_64-linux with all langs+Ada. As
>> > > it contains all the "gcc_assert (!result)" this means that the
>> > > expansion now doesn't fold anything anymore,
>> >
>> > Except in expand_builtin_interclass_mathfn, that is. I haven't
>> > tackled that one yet.
>>
>> It makes sense to do that afterwards. The patch looks very sensible.
>
> and On Wed, 23 Sep 2009, I wrote:
>
>> In a final patch I'll obviously remove all the code that is proved dead
>> with the asserts,
>
> Like this. I've also done the interclass_mathfn stuff. Instead of
> rewriting the folder in terms of UN*_EXPR directly I still use the way
> over isgreater and friends. Easier to be sure of its equivalence :)
>
> The core of the patch is the same like in
> http://gcc.gnu.org/ml/gcc-patches/2009-09/msg01644.html . The only other
> real change is this hunk of fold-const.c:
>
> + /* !exp != 0 becomes !exp */
> + if (TREE_CODE (arg0) == TRUTH_NOT_EXPR && integer_zerop (arg1)
> + && code == NE_EXPR)
> + return non_lvalue_loc (loc, fold_convert_loc (loc, type, arg0));
>
> This is needed because the new isinf() folders sometimes generate
> "!(call) != 0" and sometimes "!(call)" and then the only thing that sees
> the equivalence between them at -O0 is fold. (testcase is
> gcc.dg/torture/builtin-isinf_sign-1.c which expects that all optimizations
> also happen at -O0, and I didn't really want to argue that).
>
> The transformation is correct because the outcome of X != 0 is in {0,1},
> as is the outcome of TRUTH_NOT_EXPR(something). I also think the
> transformation is generally worthwhile.
>
> The change of gcc.dg/builtins-44.c should be obvious. It's trying to
> detect if the result of __builtin_isinf(-Inf) is -1. That guarantee holds
> only for __builtin_isinf_sign. When the latter was added (for PR35509)
> the testcase wasn't amended because it still happened to work due to pass
> ordering.
>
> The noisiness comes from deleting all (now) dead code in builtins.c, which
> actually is nice as I could remove 13 useless expanders.
>
> This patch regstrapped on x86_64-linux (all langs+Ada) and ppc64-linux
> (default32 and default64 CPU, all langs no Ada).
>
> If I'm not mistaken this should finally enable me to make the builtin
> expanders work on gimple statements in a more or less straight forward
> way.
>
> Okay for trunk?
>
>
> Ciao,
> Michael.
> --
> * builtins.c (interclass_mathfn_icode): New helper.
> (expand_builtin_interclass_mathfn): Use it here, and split folding
> into ...
> (fold_builtin_interclass_mathfn): ... this new folder.
> (build_call_nofold_loc): New static helper.
> (build_call_nofold): New wrapper macro for above.
> (expand_builtin_int_roundingfn): Use it instead of build_call_expr.
> (expand_builtin_pow): Ditto.
> (expand_builtin_memset_args): Ditto.
> (expand_builtin_printf): Ditto.
> (expand_builtin_fprintf): Ditto.
> (expand_builtin_sprintf): Ditto.
> (expand_builtin_memory_chk): Ditto.
> (expand_builtin_mempcpy_args): Ditto and don't call folders.
> (expand_builtin_stpcpy): Ditto.
> (expand_builtin_strcmp): Ditto.
> (expand_builtin_strncmp): Ditto.
> (expand_builtin_strcpy): Remove FNDECL and MODE arguments.
> (expand_builtin_strcpy_args): Don't call folders.
> (expand_builtin_memcmp): Ditto.
> (expand_builtin_strncpy): Ditto, and use target.
> (expand_builtin_memcpy): Ditto.
> (expand_builtin_strstr, expand_builtin_strchr, expand_builtin_strrchr,
> expand_builtin_strpbrk, expand_builtin_memmove,
> expand_builtin_memmove_args, expand_builtin_bcopy,
> expand_builtin_memchr, expand_builtin_strcat, expand_builtin_strncat,
> expand_builtin_strspn, expand_builtin_strcspn,
> expand_builtin_fputs): Remove these.
> (expand_builtin): Don't call the above, change calls to other
> expanders that changed prototype.
> (fold_builtin_stpcpy): New folder split out from expand_builtin_stpcpy.
> (fold_builtin_1 <ISFINITE, ISINF, ISNORMAL>): Call
> fold_builtin_interclass_mathfn.
> (fold_builtin_2 <STPCPY>): Call fold_builtin_stpcpy.
> (fold_builtin_strcat): Add folding split from expand_builtin_strcat.
>
> * fold-const.c (fold_binary_loc <NE_EXPR>): Add !exp != 0 -> !exp.
> * passes.c (init_optimization_passes): Move pass_fold_builtins
> after last phiopt pass.
> * tree-inline.c (fold_marked_statements): When folding builtins
> iterate over all instruction potentially generated.
> * tree-ssa-ccp.c (gimplify_and_update_call_from_tree): Declare
> earlier.
> (fold_gimple_call): Use it to always fold calls (into potentially
> multiple instructions).
> * tree-ssa-dom.c (optimize_stmt): Resolve __builtin_constant_p
> calls into zero at this time.
> * tree-ssa-propagate.c (substitute_and_fold): Ignore multiple
> statements generated by builtin folding.
>
> testsuite/
> * gcc.dg/builtins-44.c: Use __builtin_isinf_sign when checking
> for sign of -Inf.
>
> Index: builtins.c
> ===================================================================
> --- builtins.c (Revision 152208)
> +++ builtins.c (Arbeitskopie)
> @@ -2551,6 +2490,22 @@ expand_builtin_cexpi (tree exp, rtx targ
> target, VOIDmode, EXPAND_NORMAL);
> }
>
> +static tree
> +build_call_nofold_loc (location_t loc, tree fndecl, int n, ...)
Missing comment.
> Index: tree-inline.c
> ===================================================================
> --- tree-inline.c (Revision 152208)
> +++ tree-inline.c (Arbeitskopie)
> @@ -3873,7 +3873,36 @@ fold_marked_statements (int first, struc
> gimple old_stmt = gsi_stmt (gsi);
> tree old_decl = is_gimple_call (old_stmt) ? gimple_call_fndecl (old_stmt) : 0;
>
> - if (fold_stmt (&gsi))
> + if (old_decl && DECL_BUILT_IN (old_decl))
> + {
> + /* Folding builtins can create multiple instructions,
> + we need to look at all of them. */
> + gimple_stmt_iterator i2 = gsi;
> + gsi_prev (&i2);
> + if (fold_stmt (&gsi))
> + {
> + gimple new_stmt;
> + if (gsi_end_p (i2))
> + i2 = gsi_start_bb (BASIC_BLOCK (first));
> + else
> + gsi_next (&i2);
> + while (1)
> + {
> + new_stmt = gsi_stmt (i2);
> + update_stmt (new_stmt);
> + if (is_gimple_call (old_stmt)
The old stmt should always be a call here?
> + || is_gimple_call (new_stmt))
> + cgraph_update_edges_for_call_stmt (old_stmt, old_decl, new_stmt);
> +
> + if (maybe_clean_or_replace_eh_stmt (old_stmt, new_stmt))
> + gimple_purge_dead_eh_edges (BASIC_BLOCK (first));
If that ever happens to replace EH on not the last stmt inserted then
we would need to split the block here. Thus, this only needs to be done
on gsi_stmt (gsi).
> + if (new_stmt == gsi_stmt (gsi))
> + break;
> + gsi_next (&i2);
> + }
> + }
> + }
> + else if (fold_stmt (&gsi))
> {
> /* Re-read the statement from GSI as fold_stmt() may
> have changed it. */
A much nicer interface to fold_stmt would be
bool fold_stmt (gimple stmt, gimple_seq *seq)
where if *SEQ is not NULL after the call *SEQ replaces the old stmt,
otherwise it is updated in-place.
But that's for the long awaited cleanup of fold_stmt*
> Index: tree-ssa-dom.c
> ===================================================================
> --- tree-ssa-dom.c (Revision 152208)
> +++ tree-ssa-dom.c (Arbeitskopie)
> @@ -2133,6 +2133,20 @@ optimize_stmt (basic_block bb, gimple_st
> {
> eliminate_redundant_computations (&si);
> stmt = gsi_stmt (si);
> + if (gimple_code (stmt) == GIMPLE_CALL)
> + {
> + /* Resolve __builtin_constant_p. If it hasn't been
> + folded to integer_one_node by now, it's fairly
> + certain that the value simply isn't constant. */
> + tree callee = gimple_call_fndecl (stmt);
> + if (callee
> + && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL
> + && DECL_FUNCTION_CODE (callee) == BUILT_IN_CONSTANT_P)
> + {
> + propagate_tree_value_into_stmt (&si, integer_zero_node);
> + stmt = gsi_stmt (si);
> + }
> + }
> }
>
> /* Record any additional equivalences created by this statement. */
While I think this is reasonable please add a comment before the
scheduled pass in passes.c that this is done there to remind people
that try to (re-)move things in the pass pipeline.
Ok with these changes.
Thanks,
Richard.
More information about the Gcc-patches
mailing list