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