This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: SRA and inconsistencies in bit-field types
On Feb 19, 2007, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Feb 18, 2007, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> Or, even better, could you perhaps provide me with a testcase and
>>> instructions to trigger the problem on a uninstalled toolchain?
>> Attached. In the build directory: gcc/xgcc -Bgcc -O -S p.adb -Igcc/ada/rts
>> +===========================GNAT BUG DETECTED==============================+
>> | 4.3.0 20070216 (experimental) (i586-suse-linux-gnu) GCC error: |
>> | in sra_build_assignment, at tree-sra.c:1733 |
>> | Error detected at p.adb:11:4
> Thanks. I'm looking into it, but I haven't been able to figure out
> how to fix this yet, so I'm temporarily disabling the assertion, such
> that this testcase compiles again.
After further investigation, I've concluded this test is just too
strict, and the intent behind it just can't be met.
The problem scenario is when the front end emits accesses to fields of
a RECORD_TYPE using types that are different from the fields' nominal
types. We'll end up using these alternate types for scalarization.
Then, if there are also copies between entire records, some of which
have fields accessed with different types and are scalarized, some of
which don't or aren't scalarizable, we'll emit per-element assignments
using different types. The types are supposed to be compatible to
some extent, but their main variant types don't necessarily have the
same canonical type, and not even language-specific type compatibility
tests define as compatible (in this particular case, it's pointers to
the gimplified array data and array bound type for one operand and
pointer-to-void in the other.
Adding fold_convert to sra_build_assignment would thus fix the crash,
introducing a NOP_EXPR or CONVERT_EXPR when needed, even though
everything appears to work just fine without it, and CONVERT_EXPRs
would be quite unexpected. In fact they'd have hidden the failure
mode that got me to investigate the bug that started this thread.
I've actually written code that took note of situations in which a
field was accessed under a type other than its nominal type, or under
multiple types, and only allowed type conversions in these cases, but
I decided that was too much wasted effort. The patch I've discarded
is attached, for the record, after the patch I'm checking in, that
permanently removes the assertion checks, replacing them with comments
explaining briefly why we'd like to but can't perform the sanity
checks.
I'm going ahead and checking this in.
for gcc/ChangeLog
from Alexandre Oliva <aoliva@redhat.com>
* tree-sra.c (sra_build_assignment): Replace assertion
checking with a comment explaining why it can't be done.
Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c.orig 2007-02-19 03:50:07.000000000 -0200
+++ gcc/tree-sra.c 2007-02-19 03:54:08.000000000 -0200
@@ -1731,12 +1731,14 @@ generate_element_ref (struct sra_elt *el
static tree
sra_build_assignment (tree dst, tree src)
{
-#if 0 /* ENABLE_CHECKING */
- /* This test ought to pass, but it is unfortunately too strict for
- now. */
- gcc_assert (TYPE_CANONICAL (TYPE_MAIN_VARIANT (TREE_TYPE (dst)))
- == TYPE_CANONICAL (TYPE_MAIN_VARIANT (TREE_TYPE (src))));
-#endif
+ /* It was hoped that we could perform some type sanity checking
+ here, but since front-ends can emit accesses of fields in types
+ different from their nominal types and copy structures containing
+ them as a whole, we'd have to handle such differences here.
+ Since such accesses under different types require compatibility
+ anyway, there's little point in making tests and/or adding
+ conversions to ensure the types of src and dst are the same.
+ So we just assume type differences at this point are ok. */
return build2 (GIMPLE_MODIFY_STMT, void_type_node, dst, src);
}
This patch works, in that it enables us to add the assertion check to
compare types in the assignment, but it's totally unnecessary, and all
it appears to accomplish is to introduce unnecessary type casts
to/from void*, when fields that are accessed (and scalarized) with a
type different from its nominal type, but the enclosing structure is
copied as a whole.
Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c.orig 2007-02-19 01:51:14.000000000 -0200
+++ gcc/tree-sra.c 2007-02-19 03:40:00.000000000 -0200
@@ -147,6 +147,10 @@ struct sra_elt
/* True if there is BIT_FIELD_REF on the lhs with a vector. */
bool is_vector_lhs;
+
+ /* True if the element type differs from the type of the field, or
+ if it is ever accessed with a different type. */
+ bool may_need_conversion;
};
#define IS_ELEMENT_FOR_GROUP(ELEMENT) (TREE_CODE (ELEMENT) == RANGE_EXPR)
@@ -570,6 +574,10 @@ lookup_element (struct sra_elt *parent,
elt->type = type;
elt->is_scalar = is_sra_scalar_type (type);
+ if (TYPE_MAIN_VARIANT (TREE_TYPE (child))
+ != TYPE_MAIN_VARIANT (type))
+ elt->may_need_conversion = true;
+
if (parent)
{
if (IS_ELEMENT_FOR_GROUP (elt->element))
@@ -593,6 +601,9 @@ lookup_element (struct sra_elt *parent,
bitmap_set_bit (needs_copy_in, DECL_UID (child));
}
}
+ else if (elt && type
+ && TYPE_MAIN_VARIANT (elt->type) != TYPE_MAIN_VARIANT (type))
+ elt->may_need_conversion = true;
return elt;
}
@@ -1726,18 +1737,28 @@ generate_element_ref (struct sra_elt *el
return elt->element;
}
-/* Create an assignment statement from SRC to DST. */
+/* Create an assignment statement from SRC to DST. If CONV and the
+ types differ, add a conversion, otherwise fail. If WHERE is
+ non-null, it must be a GIMPLE_MODIFY_STMT that will be modified
+ in-place. */
static tree
-sra_build_assignment (tree dst, tree src)
+sra_build_assignment (tree dst, tree src, bool conv, tree where)
{
-#if 0 /* ENABLE_CHECKING */
- /* This test ought to pass, but it is unfortunately too strict for
- now. */
- gcc_assert (TYPE_CANONICAL (TYPE_MAIN_VARIANT (TREE_TYPE (dst)))
- == TYPE_CANONICAL (TYPE_MAIN_VARIANT (TREE_TYPE (src))));
-#endif
- return build2 (GIMPLE_MODIFY_STMT, void_type_node, dst, src);
+ if (TYPE_MAIN_VARIANT (TREE_TYPE (dst))
+ != TYPE_MAIN_VARIANT (TREE_TYPE (src)))
+ {
+ gcc_assert (conv);
+ src = fold_convert (TREE_TYPE (dst), src);
+ }
+
+ if (!where)
+ return build2 (GIMPLE_MODIFY_STMT, void_type_node, dst, src);
+
+ GIMPLE_STMT_OPERAND (where, 0) = dst;
+ GIMPLE_STMT_OPERAND (where, 1) = src;
+ update_stmt (where);
+ return where;
}
/* Generate a set of assignment statements in *LIST_P to copy all
@@ -1756,23 +1777,28 @@ generate_copy_inout (struct sra_elt *elt
&& TREE_CODE (TREE_TYPE (expr)) == COMPLEX_TYPE)
{
tree r, i;
+ bool conv;
c = lookup_element (elt, integer_zero_node, NULL, NO_INSERT);
r = c->replacement;
+ conv = c->may_need_conversion;
c = lookup_element (elt, integer_one_node, NULL, NO_INSERT);
i = c->replacement;
+ conv = conv || c->may_need_conversion;
t = build2 (COMPLEX_EXPR, elt->type, r, i);
- t = sra_build_assignment (expr, t);
+ t = sra_build_assignment (expr, t, conv, NULL);
SSA_NAME_DEF_STMT (expr) = t;
append_to_statement_list (t, list_p);
}
else if (elt->replacement)
{
if (copy_out)
- t = sra_build_assignment (elt->replacement, expr);
+ t = sra_build_assignment (elt->replacement, expr,
+ elt->may_need_conversion, NULL);
else
- t = sra_build_assignment (expr, elt->replacement);
+ t = sra_build_assignment (expr, elt->replacement,
+ elt->may_need_conversion, NULL);
append_to_statement_list (t, list_p);
}
else
@@ -1807,7 +1833,9 @@ generate_element_copy (struct sra_elt *d
gcc_assert (src->replacement);
- t = sra_build_assignment (dst->replacement, src->replacement);
+ t = sra_build_assignment (dst->replacement, src->replacement,
+ dst->may_need_conversion
+ || src->may_need_conversion, NULL);
append_to_statement_list (t, list_p);
}
}
@@ -1838,7 +1866,8 @@ generate_element_zero (struct sra_elt *e
gcc_assert (elt->is_scalar);
t = fold_convert (elt->type, integer_zero_node);
- t = sra_build_assignment (elt->replacement, t);
+ t = sra_build_assignment (elt->replacement, t,
+ elt->may_need_conversion, NULL);
append_to_statement_list (t, list_p);
}
}
@@ -1850,7 +1879,7 @@ static void
generate_one_element_init (tree var, tree init, tree *list_p)
{
/* The replacement can be almost arbitrarily complex. Gimplify. */
- tree stmt = sra_build_assignment (var, init);
+ tree stmt = sra_build_assignment (var, init, false, NULL);
gimplify_and_add (stmt, list_p);
}
@@ -2099,9 +2128,11 @@ scalarize_copy (struct sra_elt *lhs_elt,
RETURN_EXPR, and why we should never see one here. */
gcc_assert (TREE_CODE (stmt) == GIMPLE_MODIFY_STMT);
- GIMPLE_STMT_OPERAND (stmt, 0) = lhs_elt->replacement;
- GIMPLE_STMT_OPERAND (stmt, 1) = rhs_elt->replacement;
- update_stmt (stmt);
+ sra_build_assignment (lhs_elt->replacement,
+ rhs_elt->replacement,
+ lhs_elt->may_need_conversion
+ || rhs_elt->may_need_conversion,
+ stmt);
}
else if (lhs_elt->use_block_copy || rhs_elt->use_block_copy)
{
--
Alexandre Oliva http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member http://www.fsfla.org/
Red Hat Compiler Engineer aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist oliva@{lsd.ic.unicamp.br, gnu.org}