This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, PR 42025] Really ensuring type compatibilty in IL generated by IPA-SRA
Hi,
On Fri, Nov 20, 2009 at 03:59:41PM +0100, Richard Guenther wrote:
> On Fri, 20 Nov 2009, Martin Jambor wrote:
>
> > > This doesn't look correct. Above you may end up setting dont_convert
> > > for the LHS where you may not fold the VIEW_CONVERT_EXPR you generate
> > > (because it might end up being a NOP_EXPR which is invalid on the LHS).
> >
> > Thanks for the comment, this might indeed probably be a problem.
> >
> > Is it always invalid on the LHS or only when the argument is not a
> > gimple register (for example when it has DECL_GIMPLE_REG_P == 0 which
> > complex replacements would currently have)? I thought this was only
> > problem if the conversion argument was an SSA_NAME.
>
> A NOP_EXPR on the LHS is always invalid (but was that your question?).
> Fold does not distinguish between SSA_NAMEs or bare decls and
> happily folds V_C_E<signed int>(unsigned int) to a NOP_EXPR independent
> of how the operand looks like (maybe that was it? ;)).
Well, I was surprised I could still encounter a NOP_EXPR at this
stage in the first place. Then I also thought that if I could it would
be appropriate wherever a V_C_E would. But apparently I was wrong on
both accounts. I really haven't seen one in months...
>
> > > A union with a complex signed integer entry and a struct with
> > > two unsigned ints and a REALPART_EXPR on it should trigger that
> > > (well, if carefully crafted).
> >
> > The problem may not occur with a union of a complex and a structure
> > because the comparison function compare_access_positions we use for
> > sorting always prefers gimple register types over aggregates, so the
> > selected type of the replacements will always be gimple register type
> > (if any is involved and complex numbers are gimple register types).
> > Nevertheless, the conversion could still happen if we had a union of
> > two incompatible gimple registers of the same size and at least one of
> > them was a complex number.
> >
> > So I was thinking, if conversions of non-gimple registers were ok on
> > the left hand side, we could change compare_access_positions to prefer
> > complex types over the other gimple types. That way, the
> > replacement would always have DECL_GIMPLE_REG_P equal to zero.
> >
> > But I guess it all depends on the first question. If conversions on
> > the LHS are a problem even for non-gimple registers, I will probably
> > disable IPA-SRA for parameters which have an access group with a
> > partial lhs access and multiple different types.
>
> That sounds like overkill. Why not avoid folding at all and not keep the
> build1 (...) as it was if we process the LHS?
>
Sure, that is easy to do, a patch (bootstrapped and tested on
x86_64-linux) is below.
Thanks,
Martin
2009-11-20 Martin Jambor <mjambor@suse.cz>
PR middle-end/42025
* tree-sra.c (access_precludes_ipa_sra_p): New function.
(splice_param_accesses): Check all accesses by calling
access_precludes_ipa_sra_p.
(sra_ipa_modify_expr): Rename argument erite to dont_convert and do
not convert types if it is true.
(sra_ipa_modify_assign): Convert types in case of mismatch.
* testsuite/gcc.c-torture/compile/pr42025-1.c: New test.
* testsuite/gcc.c-torture/compile/pr42025-2.c: New test.
Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -3042,6 +3042,27 @@ unmodified_by_ref_scalar_representative
return repr;
}
+/* Return true iff this access precludes IPA-SRA of the parameter it is
+ associated with. */
+
+static bool
+access_precludes_ipa_sra_p (struct access *access)
+{
+ /* Avoid issues such as the second simple testcase in PR 42025. The problem
+ is incompatible assign in a call statement (and possibly even in asm
+ statements). This can be relaxed by using a new temporary but only for
+ non-TREE_ADDRESSABLE types and is probably not worth the complexity. (In
+ intraprocedural SRA we deal with this by keeping the old aggregate around,
+ something we cannot do in IPA-SRA.) */
+ if (access->write
+ && (is_gimple_call (access->stmt)
+ || gimple_code (access->stmt) == GIMPLE_ASM))
+ return true;
+
+ return false;
+}
+
+
/* Sort collected accesses for parameter PARM, identify representatives for
each accessed region and link them together. Return NULL if there are
different but overlapping accesses, return the special ptr value meaning
@@ -3073,6 +3094,8 @@ splice_param_accesses (tree parm, bool *
bool modification;
access = VEC_index (access_p, access_vec, i);
modification = access->write;
+ if (access_precludes_ipa_sra_p (access))
+ return NULL;
/* Access is about to become group representative unless we find some
nasty overlap which would preclude us from breaking this parameter
@@ -3093,6 +3116,9 @@ splice_param_accesses (tree parm, bool *
else if (ac2->size != access->size)
return NULL;
+ if (access_precludes_ipa_sra_p (ac2))
+ return NULL;
+
modification |= ac2->write;
ac2->group_representative = access;
ac2->next_sibling = access->next_sibling;
@@ -3523,13 +3549,19 @@ replace_removed_params_ssa_names (gimple
return true;
}
-/* Callback for scan_function. If the expression *EXPR should be replaced by a
- reduction of a parameter, do so. DATA is a pointer to a vector of
- adjustments. */
+/* Callback for scan_function and helper to sra_ipa_modify_assign. If the
+ expression *EXPR should be replaced by a reduction of a parameter, do so.
+ DATA is a pointer to a vector of adjustments. DONT_CONVERT specifies
+ whether the function should care about type incompatibility the current and
+ new expressions. If it is true, the function will leave incompatibility
+ issues to the caller.
+
+ When called directly by scan_function, DONT_CONVERT is true when the EXPR is
+ a write (LHS) expression. */
static bool
sra_ipa_modify_expr (tree *expr, gimple_stmt_iterator *gsi ATTRIBUTE_UNUSED,
- bool write ATTRIBUTE_UNUSED, void *data)
+ bool dont_convert, void *data)
{
ipa_parm_adjustment_vec adjustments;
int i, len;
@@ -3543,10 +3575,10 @@ sra_ipa_modify_expr (tree *expr, gimple_
if (TREE_CODE (*expr) == BIT_FIELD_REF
|| TREE_CODE (*expr) == IMAGPART_EXPR
|| TREE_CODE (*expr) == REALPART_EXPR)
- expr = &TREE_OPERAND (*expr, 0);
- while (TREE_CODE (*expr) == NOP_EXPR
- || TREE_CODE (*expr) == VIEW_CONVERT_EXPR)
- expr = &TREE_OPERAND (*expr, 0);
+ {
+ expr = &TREE_OPERAND (*expr, 0);
+ dont_convert = false;
+ }
base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
if (!base || size == -1 || max_size == -1)
@@ -3594,13 +3626,14 @@ sra_ipa_modify_expr (tree *expr, gimple_
fprintf (dump_file, "\n");
}
- if (!useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
+ if (!dont_convert &&
+ !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
{
tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
*expr = vce;
}
- else
- *expr = src;
+ else
+ *expr = src;
return true;
}
@@ -3608,20 +3641,37 @@ sra_ipa_modify_expr (tree *expr, gimple_
essentially the same function like sra_ipa_modify_expr. */
static enum scan_assign_result
-sra_ipa_modify_assign (gimple *stmt_ptr,
- gimple_stmt_iterator *gsi ATTRIBUTE_UNUSED, void *data)
+sra_ipa_modify_assign (gimple *stmt_ptr, gimple_stmt_iterator *gsi, void *data)
{
gimple stmt = *stmt_ptr;
- bool any = false;
+ tree *lhs_p, *rhs_p;
+ bool any;
if (!gimple_assign_single_p (stmt))
return SRA_SA_NONE;
- any |= sra_ipa_modify_expr (gimple_assign_rhs1_ptr (stmt), gsi, false,
- data);
- any |= sra_ipa_modify_expr (gimple_assign_lhs_ptr (stmt), gsi, true, data);
+ rhs_p = gimple_assign_rhs1_ptr (stmt);
+ lhs_p = gimple_assign_lhs_ptr (stmt);
+
+ any = sra_ipa_modify_expr (rhs_p, gsi, true, data);
+ any |= sra_ipa_modify_expr (lhs_p, gsi, true, data);
+ if (any)
+ {
+ if (!useless_type_conversion_p (TREE_TYPE (*lhs_p), TREE_TYPE (*rhs_p)))
+ {
+ location_t loc = gimple_location (stmt);
+ tree vce = fold_build1_loc (loc, VIEW_CONVERT_EXPR,
+ TREE_TYPE (*lhs_p), *rhs_p);
+ tree tmp = force_gimple_operand_gsi (gsi, vce, true, NULL_TREE,
+ true, GSI_SAME_STMT);
+
+ gimple_assign_set_rhs_from_tree (gsi, tmp);
+ }
+
+ return SRA_SA_PROCESSED;
+ }
- return any ? SRA_SA_PROCESSED : SRA_SA_NONE;
+ return SRA_SA_NONE;
}
/* Call gimple_debug_bind_reset_value on all debug statements describing
Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42025-1.c
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gcc.c-torture/compile/pr42025-1.c
@@ -0,0 +1,24 @@
+typedef void* Ptr;
+
+struct A
+{
+ int i;
+ union
+ {
+ Ptr p;
+ char *q;
+ } u;
+};
+
+static void foo(struct A *p, char *q)
+{
+ if (p->i)
+ p->u.p = 0;
+ else
+ p->u.q = q;
+}
+
+void bar(struct A *p, char *q)
+{
+ foo(p, q);
+}
Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42025-2.c
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gcc.c-torture/compile/pr42025-2.c
@@ -0,0 +1,32 @@
+typedef struct
+{
+ void *p;
+} Ptr;
+
+struct A
+{
+ int i;
+ union
+ {
+ Ptr p;
+ char *q;
+ } u;
+};
+
+extern Ptr get_stuff (void);
+extern void use_stuff (char *);
+
+static void foo(struct A p, char *q)
+{
+ if (p.i)
+ p.u.p = get_stuff ();
+ else
+ p.u.q = q;
+
+ use_stuff (p.u.q);
+}
+
+void bar(struct A *p, char *q)
+{
+ foo(*p, q);
+}