This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR34029, constrain is_gimple_min_invariant for ADDR_EXPRs
On Thu, 3 Jan 2008, Richard Guenther wrote:
> On Jan 3, 2008 10:33 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > > To not allow &a[4 - &b] as invariant, which it is because &b is invariant
> > > and the whole address thus is. But this is not something we want to say
> > > is gimple.
> >
> > We already had this discussion 5 months ago:
> > http://gcc.gnu.org/ml/gcc/2007-07/msg00070.html
> >
> > Here is Diego's proposal:
> > http://gcc.gnu.org/ml/gcc/2007-07/msg00097.html
> >
> > Here is what you eventually approved to solve the problem:
> > http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01028.html
>
> Hm, right - now I remember.
>
> > Does your problem arise during initial gimplification or later?
>
> It occurs during VRP where SCEV analysis creates this invariant and VRP
> uses it as single-valued range.
>
> I'll see where I need to stick the new predicate.
Ok, so it's all not that simple. The invalid stmt is generated from
substitute_and_fold. For propagation into PHI arguments we can easily
check valid_gimple_expression_p - though even that is not constrained
enough, as we may only have gimple_val PHI arguments.
Now, for propagating into regular stmts we eventually need to verify
the final stmt for if it is correct gimple (or more costly, re-gimplify
it) - but doing so reveals that we for example do not detect
(long int) &s[3 - (<unnamed-signed:64>) &t]
as not valid_gimple_expression_p, because we again use (indirectly)
is_gimple_min_invariant () in that function to verify the argument
of the NOP_EXPR is valid gimple. So, instead we introduce a new
valid_gimple_val_p for its purpose that recurses into a
valid_gimple_invariant_p. Still we have the problem that restoring
the stmt to the original one is difficult (if not impossible)
-- and simply checking the
to substitute value is not enough if you consider
tmp_1 = 3 - (size_t) &t
tmp_2 = &b[tmp_1]
as the tmp_1 rhs is a valid gimple expression, but if we substitute
it to tmp_2 the result is not.
So - why again do we want is_gimple_min_invariant to accept non-gimple
invariants?
Which means - I'm back to the patch I originally proposed. (Current
work in progress for the approach above attached)
Diego?
Thanks,
Richard.
Index: tree-ssa-propagate.c
===================================================================
*** tree-ssa-propagate.c (revision 131302)
--- tree-ssa-propagate.c (working copy)
*************** get_rhs (tree stmt)
*** 571,576 ****
--- 571,616 ----
}
}
+ static bool
+ valid_gimple_invariant_p (tree expr)
+ {
+ if (!is_gimple_min_invariant (expr))
+ return false;
+
+ if (TREE_CODE (expr) == ADDR_EXPR)
+ {
+ tree t = TREE_OPERAND (expr, 0);
+ while (handled_component_p (t))
+ {
+ /* Invariant ADDR_EXPRs may only contain constant indices
+ within array references. */
+ if ((TREE_CODE (t) == ARRAY_REF
+ || TREE_CODE (t) == ARRAY_RANGE_REF)
+ && TREE_CODE (TREE_OPERAND (t, 1)) != INTEGER_CST)
+ return false;
+ t = TREE_OPERAND (t, 0);
+ }
+ }
+
+ return true;
+ }
+
+ static bool
+ valid_gimple_val_p (tree t)
+ {
+ /* Make loads from volatiles and memory vars explicit. */
+ if (is_gimple_variable (t)
+ && is_gimple_reg_type (TREE_TYPE (t))
+ && !is_gimple_reg (t))
+ return false;
+
+ /* FIXME make these decls. That can happen only when we expose the
+ entire landing-pad construct at the tree level. */
+ if (TREE_CODE (t) == EXC_PTR_EXPR || TREE_CODE (t) == FILTER_EXPR)
+ return true;
+
+ return (is_gimple_variable (t) || valid_gimple_invariant_p (t));
+ }
/* Return true if EXPR is a valid GIMPLE expression. */
*************** valid_gimple_expression_p (tree expr)
*** 591,603 ****
case tcc_binary:
case tcc_comparison:
! if (!is_gimple_val (TREE_OPERAND (expr, 0))
! || !is_gimple_val (TREE_OPERAND (expr, 1)))
return false;
break;
case tcc_unary:
! if (!is_gimple_val (TREE_OPERAND (expr, 0)))
return false;
break;
--- 631,643 ----
case tcc_binary:
case tcc_comparison:
! if (!valid_gimple_val_p (TREE_OPERAND (expr, 0))
! || !valid_gimple_val_p (TREE_OPERAND (expr, 1)))
return false;
break;
case tcc_unary:
! if (!valid_gimple_val_p (TREE_OPERAND (expr, 0)))
return false;
break;
*************** valid_gimple_expression_p (tree expr)
*** 612,618 ****
/* ??? More checks needed, see the GIMPLE verifier. */
if ((TREE_CODE (t) == ARRAY_REF
|| TREE_CODE (t) == ARRAY_RANGE_REF)
! && !is_gimple_val (TREE_OPERAND (t, 1)))
return false;
t = TREE_OPERAND (t, 0);
}
--- 652,658 ----
/* ??? More checks needed, see the GIMPLE verifier. */
if ((TREE_CODE (t) == ARRAY_REF
|| TREE_CODE (t) == ARRAY_RANGE_REF)
! && !valid_gimple_val_p (TREE_OPERAND (t, 1)))
return false;
t = TREE_OPERAND (t, 0);
}
*************** valid_gimple_expression_p (tree expr)
*** 622,636 ****
}
case TRUTH_NOT_EXPR:
! if (!is_gimple_val (TREE_OPERAND (expr, 0)))
return false;
break;
case TRUTH_AND_EXPR:
case TRUTH_XOR_EXPR:
case TRUTH_OR_EXPR:
! if (!is_gimple_val (TREE_OPERAND (expr, 0))
! || !is_gimple_val (TREE_OPERAND (expr, 1)))
return false;
break;
--- 662,676 ----
}
case TRUTH_NOT_EXPR:
! if (!valid_gimple_val_p (TREE_OPERAND (expr, 0)))
return false;
break;
case TRUTH_AND_EXPR:
case TRUTH_XOR_EXPR:
case TRUTH_OR_EXPR:
! if (!valid_gimple_val_p (TREE_OPERAND (expr, 0))
! || !valid_gimple_val_p (TREE_OPERAND (expr, 1)))
return false;
break;
*************** replace_phi_args_in (tree phi, prop_valu
*** 1095,1101 ****
{
tree val = prop_value[SSA_NAME_VERSION (arg)].value;
! if (val && val != arg && may_propagate_copy (arg, val))
{
if (TREE_CODE (val) != SSA_NAME)
prop_stats.num_const_prop++;
--- 1135,1146 ----
{
tree val = prop_value[SSA_NAME_VERSION (arg)].value;
! if (val && val != arg
! && may_propagate_copy (arg, val)
! /* We need to avoid propagating non-gimple but invariant
! expressions into PHI arguments. */
! && is_gimple_val (val)
! && valid_gimple_expression_p (val))
{
if (TREE_CODE (val) != SSA_NAME)
prop_stats.num_const_prop++;
*************** substitute_and_fold (prop_value_t *prop_
*** 1231,1238 ****
folded. */
did_replace = false;
replaced_address = false;
! if (dump_file && (dump_flags & TDF_DETAILS))
! prev_stmt = unshare_expr (stmt);
/* If we have range information, see if we can fold
predicate expressions. */
--- 1276,1287 ----
folded. */
did_replace = false;
replaced_address = false;
! /* if (dump_file && (dump_flags & TDF_DETAILS)) */
! /* XXX If we unshare the expression so we can restore it later we
! break all on-the-side structs and get wrong BB, etc.
! If we do not unshare it, the original copy get's modified as well
! and we cannot restore it either. Bah. */
! prev_stmt = /* unshare_expr */ (stmt);
/* If we have range information, see if we can fold
predicate expressions. */
*************** substitute_and_fold (prop_value_t *prop_
*** 1268,1273 ****
--- 1317,1331 ----
tree_purge_dead_eh_edges (bb);
rhs = get_rhs (stmt);
+ /* If the substitution and simplification did not result
+ in valid GIMPLE, restore the original stmt. */
+ if (!set_rhs (&stmt, rhs))
+ {
+ *(bsi_stmt_ptr (i)) = prev_stmt;
+ discard_stmt_changes (bsi_stmt_ptr (i));
+ }
+ else
+ {
if (TREE_CODE (rhs) == ADDR_EXPR)
recompute_tree_invariant_for_addr_expr (rhs);
*************** substitute_and_fold (prop_value_t *prop_
*** 1283,1288 ****
--- 1341,1347 ----
/* Determine what needs to be done to update the SSA form. */
pop_stmt_changes (bsi_stmt_ptr (i));
something_changed = true;
+ }
}
else
{