This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [tuples][patch] Convert pass_fold_builtins to tuples and related fixes
On Thu, Mar 13, 2008 at 1:33 PM, Diego Novillo <dnovillo@google.com> wrote:
> On 3/12/08 9:53 PM, Bill Maddox wrote:
>
> > +static tree
> > +gimple_fold_builtin_varargs (tree fndecl, gimple stmt, bool ignore
> > ATTRIBUTE_UNUSED)
>
> If 'ignore' is not used, better not have it at all.
I made this change, though it was copied from existing code. There are other
functions like fold_builtin_n which take the ignore argument and use
it. I think
the intent of the original code was to maintain a consistent protocol for these.
They dispatch to handlers for various builtins, some of which may care about
ignored results, and others may not. It simply happens that, at the moment,
none the of varargs folders care about it.
> > /* The RHS of the statement defining VAR must either have a
> > constant length or come from another SSA_NAME with a constant
> > length. */
> > - if (gimple_num_ops (def_stmt) == 2)
> > + if (get_gimple_rhs_class (gimple_subcode (def_stmt))
> > + == GIMPLE_SINGLE_RHS)
>
> Use gimple_assign_copy_p() here.
Note that I already know that I'm dealing with an assignment, so this test
is simpler and corresponds better with the pre-tuples logic. If you'd prefer
gimple_assign_copy_p(), I can do it that way. The test here is an idiom that
I've frequently used, however, so there are others floating around.
> > + for (gsi_next (&i); !gsi_end_p (i); gsi_next (&i))
> > + {
> > + stmt = gsi_stmt (i);
> > + if (gimple_code (stmt) == GIMPLE_ASM)
> > + return false;
>
> This function returns 'tree'. You need 'return NULL_TREE' here.
> Likewise all the other return false in the body.
Oops! At one point, I had considered making this function return a boolean.
The tree result carries almost no information, but it works out slightly simpler
keeping it as it was. Loose C typecheckign strikes again...
> > @@ -1183,6 +1183,7 @@ substitute_and_fold (prop_value_t *prop_
> > if (use_ranges_p)
> > did_replace = fold_predicate_in (stmt);
> >
> > + /* NOTE: The boolean variable REPLACED_ADDRESS is never used. */
>
> Ah, good catch. Let's remove it and make replace_uses_in() static then.
> This is something that can be moved to mainline now. Could you send a
> mainline patch for it?
I put the comment in because I was concerned that perhaps it *should* have been
used, perhaps in connection with the calls to
recompute_tree_invariant_for_add_expr().
If we are satisfied with the non-use, however, I'll gladly rip it out.
> > @@ -797,9 +797,11 @@ find_new_referenced_vars_1 (tree *tp, in
> > }
> >
> > void
> > -find_new_referenced_vars (tree *stmt_p)
> > +find_new_referenced_vars (gimple_stmt_iterator *gsi)
> > {
> > - walk_tree (stmt_p, find_new_referenced_vars_1, NULL, NULL);
> > + /* Invoke callback on each operand, in both the statement
> > + referenced by GSI and in any embedded statements. */
> > + walk_gimple_stmt (gsi, NULL, find_new_referenced_vars_1, NULL);
>
> Easier to call walk_gimple_op here.
Is this equivalent? It looks to me that walk_gimple_op does not visit embedded
statements. The old code did this. Also, I need to take a GSI as an argument,
though I can easily grab the statement for walk_gimple_op with a gsi_stmt().
> > @@ -2722,6 +2722,8 @@ gimple_return_set_retval (gimple gs, tre
> > {
> > GIMPLE_CHECK (gs, GIMPLE_RETURN);
> > gcc_assert (gs->with_ops.num_ops == 1);
> > + /* FIXME tuples. The check in gimple_build_return
> > + is more restrictive. Which one is correct? */
> > gcc_assert (is_gimple_operand (retval));
>
> My bug. Change this to the gimple_build_return() check.
Done. I moved the check to gimple_return_set_retval, and
gimple_build_return now gets the check when it calls
gimple_return_set_retval. It is now possible to set the
return value to NULL. Previously, a no-value GIMPLE_RETURN
could be constructed afresh, but it was not possible to null out
an existing result, as we have been able to do for GIMPLE_CALL.
--Bill