From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17290 invoked by alias); 30 Nov 2009 14:15:38 -0000 Received: (qmail 17267 invoked by uid 22791); 30 Nov 2009 14:15:36 -0000 X-SWARE-Spam-Status: No, hits=-7.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 30 Nov 2009 14:15:29 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id A94EC74609 for ; Mon, 30 Nov 2009 15:15:26 +0100 (CET) Date: Mon, 30 Nov 2009 14:18:00 -0000 From: Richard Guenther To: Martin Jambor Cc: GCC Patches Subject: Re: [PATCH, SRA, PR 42196] Dealing with partial accesses to complex numbers in unions In-Reply-To: <20091130141122.GC12510@virgil.suse.cz> Message-ID: References: <20091130141122.GC12510@virgil.suse.cz> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2009-11/txt/msg01694.txt.bz2 On Mon, 30 Nov 2009, Martin Jambor wrote: > Hi, > > a recent IPA-SRA bug made me try a to access individual components of > complex numbers in unions in a few different ways and they have all > failed nicely (PR 42196). This patch hopefully fixes all such issues > by doing two things: > > First, if a part of an aggregate is accessed as both a complex number > or a vector and a different gimple register type, the comparison > function picks the vector or the complex type. The reason for this is > that if a replacement is written to component-wise (through > REALPART_EXPR or IMAGPART_EXPR) it must not be a gimple register > despite being non-addressable and of a register type and this is > expected and already works only for complex and vector types. > > The above alone fixes the first testcase but not the other two. Thus > the second change makes complex expressions (which are either > component-wise or not in a gimple assignment statement) go through the > same path that we invented for one field structures, namely through > the original aggregate. The alternative would be introducing a number > of type conversions and getting all the situations right wouldn't > probably be worth it. > > However, in order not to hurt complex numbers which do not need any > typecasts, I only do it when the access group is accessed through more > than one distinct type. To facilitate this, I have added yet another > flag to the access structure. > > Bootstrapped and tested on x86_64-linux (but without Ada because that > did not build this morning). OK for trunk? > > Thanks, > > Martin > > > 2009-11-27 Martin Jambor > > PR middle-end/42196 > * tree-sra.c (struct access): New field grp_different_types. > (dump_access): Dump grp_different_types. > (compare_access_positions): Prefer scalars and vectors over other > scalar types. > (sort_and_splice_var_accesses): Set grp_different_types if appropriate. > (sra_modify_expr): Use the original also when dealing with a complex > or vector group accessed as multiple types. > > * testsuite/gcc.c-torture/compile/pr42196-1.c: New test. > * testsuite/gcc.c-torture/compile/pr42196-2.c: New test. > * testsuite/gcc.c-torture/compile/pr42196-3.c: New test. > > > Index: mine/gcc/tree-sra.c > =================================================================== > --- mine.orig/gcc/tree-sra.c > +++ mine/gcc/tree-sra.c > @@ -199,6 +199,10 @@ struct access > BIT_FIELD_REF? */ > unsigned grp_partial_lhs : 1; > > + /* Does this group contain accesses to different types? (I.e. through a union > + or a similar mechanism). */ > + unsigned grp_different_types : 1; > + > /* Set when a scalar replacement should be created for this variable. We do > the decision and creation at different places because create_tmp_var > cannot be called from within FOR_EACH_REFERENCED_VAR. */ > @@ -339,12 +343,14 @@ dump_access (FILE *f, struct access *acc > fprintf (f, ", grp_write = %d, grp_read = %d, grp_hint = %d, " > "grp_covered = %d, grp_unscalarizable_region = %d, " > "grp_unscalarized_data = %d, grp_partial_lhs = %d, " > - "grp_to_be_replaced = %d\n grp_maybe_modified = %d, " > + "grp_different_types = %d, grp_to_be_replaced = %d, " > + "grp_maybe_modified = %d, " > "grp_not_necessarilly_dereferenced = %d\n", > access->grp_write, access->grp_read, access->grp_hint, > access->grp_covered, access->grp_unscalarizable_region, > access->grp_unscalarized_data, access->grp_partial_lhs, > - access->grp_to_be_replaced, access->grp_maybe_modified, > + access->grp_different_types, access->grp_to_be_replaced, > + access->grp_maybe_modified, > access->grp_not_necessarilly_dereferenced); > else > fprintf (f, ", write = %d, grp_partial_lhs = %d\n", access->write, > @@ -1112,14 +1118,25 @@ compare_access_positions (const void *a, > { > /* Put any non-aggregate type before any aggregate type. */ > if (!is_gimple_reg_type (f1->type) > - && is_gimple_reg_type (f2->type)) > + && is_gimple_reg_type (f2->type)) > return 1; > else if (is_gimple_reg_type (f1->type) > && !is_gimple_reg_type (f2->type)) > return -1; > + /* Put any complex or vector type before any other scalar type. */ > + else if (TREE_CODE (f1->type) != COMPLEX_TYPE > + && TREE_CODE (f1->type) != VECTOR_TYPE > + && (TREE_CODE (f2->type) == COMPLEX_TYPE > + || TREE_CODE (f1->type) == VECTOR_TYPE)) typo? You probably mean f2->type here. > + return 1; > + else if ((TREE_CODE (f1->type) == COMPLEX_TYPE > + || TREE_CODE (f1->type) == VECTOR_TYPE) > + && TREE_CODE (f2->type) != COMPLEX_TYPE > + && TREE_CODE (f2->type) != VECTOR_TYPE) > + return -1; > /* Put the integral type with the bigger precision first. */ > else if (INTEGRAL_TYPE_P (f1->type) > - && INTEGRAL_TYPE_P (f2->type)) > + && INTEGRAL_TYPE_P (f2->type)) > return TYPE_PRECISION (f1->type) > TYPE_PRECISION (f2->type) ? -1 : 1; > /* Put any integral type with non-full precision last. */ > else if (INTEGRAL_TYPE_P (f1->type) > @@ -1417,6 +1434,7 @@ sort_and_splice_var_accesses (tree var) > bool grp_read = !access->write; > bool multiple_reads = false; > bool grp_partial_lhs = access->grp_partial_lhs; > + bool grp_different_types = false; > bool first_scalar = is_gimple_reg_type (access->type); > bool unscalarizable_region = access->grp_unscalarizable_region; > > @@ -1448,6 +1466,7 @@ sort_and_splice_var_accesses (tree var) > grp_read = true; > } > grp_partial_lhs |= ac2->grp_partial_lhs; > + grp_different_types |= access->type != ac2->type; !types_compatible_p (access->type, ac2->type); > unscalarizable_region |= ac2->grp_unscalarizable_region; > relink_to_new_repr (access, ac2); > > @@ -1466,6 +1485,7 @@ sort_and_splice_var_accesses (tree var) > access->grp_read = grp_read; > access->grp_hint = multiple_reads; > access->grp_partial_lhs = grp_partial_lhs; > + access->grp_different_types = grp_different_types; > access->grp_unscalarizable_region = unscalarizable_region; > if (access->first_link) > add_access_to_work_queue (access); > @@ -2112,8 +2132,15 @@ sra_modify_expr (tree *expr, gimple_stmt > access expression to extract the scalar component afterwards. > This happens if scalarizing a function return value or parameter > like in gcc.c-torture/execute/20041124-1.c, 20050316-1.c and > - gcc.c-torture/compile/20011217-1.c. */ > - if (!is_gimple_reg_type (type)) > + gcc.c-torture/compile/20011217-1.c. > + > + We also want to use this when accessing a complex or vector which can > + be accessed as a different type too, potentially creating a need for > + type conversion (see PR42196). */ > + if (!is_gimple_reg_type (type) > + || (access->grp_different_types > + && (TREE_CODE (type) == COMPLEX_TYPE > + ||TREE_CODE (type) == VECTOR_TYPE))) space missing after || ok with that changes. I'm not entirely happy with all the exceptions but for 4.5 fixing the bugs is most important. Thanks, Richard. > { > tree ref = access->base; > bool ok; > Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42196-1.c > =================================================================== > --- /dev/null > +++ mine/gcc/testsuite/gcc.c-torture/compile/pr42196-1.c > @@ -0,0 +1,28 @@ > +union U > +{ > + double d; > + __complex__ int c; > +}; > + > +double gd; > +extern double bar (union U); > + > +double foo (int b, double d, int c1, int c2) > +{ > + union U u; > + double r; > + > + if (b) > + { > + u.d = d; > + r = u.d; > + } > + else > + { > + __real__ u.c = c1; > + __imag__ u.c = c2; > + r = bar (u); > + } > + > + return r; > +} > Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42196-2.c > =================================================================== > --- /dev/null > +++ mine/gcc/testsuite/gcc.c-torture/compile/pr42196-2.c > @@ -0,0 +1,28 @@ > +union U > +{ > + __complex__ int ci; > + __complex__ float cf; > +}; > + > +float gd; > +extern float bar (union U); > + > +float foo (int b, double f1, double f2, int c1, int c2) > +{ > + union U u; > + double r; > + > + if (b) > + { > + __real__ u.cf = f1; > + __imag__ u.cf = f2; > + } > + else > + { > + __real__ u.ci = c1; > + __imag__ u.ci = c2; > + } > + > + r = bar (u); > + return r; > +} > Index: mine/gcc/testsuite/gcc.c-torture/compile/pr42196-3.c > =================================================================== > --- /dev/null > +++ mine/gcc/testsuite/gcc.c-torture/compile/pr42196-3.c > @@ -0,0 +1,27 @@ > +union U > +{ > + __complex__ int ci; > + __complex__ float cf; > +}; > + > +float gd; > +extern float bar (float, float); > + > +float foo (int b, union U u) > +{ > + float f1, f2, r; > + > + if (b) > + { > + f1 = __real__ u.cf; > + f1 = __imag__ u.cf; > + } > + else > + { > + f1 = __real__ u.ci; > + f1 = __imag__ u.ci; > + } > + > + r = bar (f1, f2); > + return r; > +}