This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Replace a SRA FIXME with an assert
On Tue, 20 Mar 2012, Martin Jambor wrote:
> Hi,
>
> On Tue, Mar 20, 2012 at 04:08:31PM +0100, Richard Guenther wrote:
> > On Tue, 20 Mar 2012, Martin Jambor wrote:
> >
> > > Hi,
> > >
> > > this patch which removes one of only two FIXMEs in tree-sra.c has been
> > > sitting in my patch queue for over a year. Yesterday I noticed it
> > > there, bootstrapped and tested it on x86_64-linux and it passed.
> > >
> > > I'd like to either commit it or just remove the comment, if there
> > > likely still are size inconsistencies in assignments but we are not
> > > planning to do anything with them in foreseeable future (and perhaps
> > > add a note to the bug).
> > >
> > > So, which should it be?
> >
> > Well. Aggregate assignments can still be off I think, especially
> > because of the disconnect between TYPE_SIZE and DECL_SIZE in
> > some cases, considering *p = x; with typeof (x) == typeof (*p)
> > (tail-padding re-use).
> >
> > The comments in PR40058 hint at that that issue might be fixed,
> > but I also remember issues with Ada.
>
> The other FIXME in tree-sra.c suggests that Ada can produce
> VIEW_CONVERT_EXPRs with a different size than its argument, perhaps
> that is it (I'll try removing that one too).
Yeah, it does that.
> >
> > GIMPLE verification ensures compatible types (but not a match
> > of type_size / decl_size which will be exposed by get_ref_base_and_extent)
> >
> > But the real question is what do you want to guard against here?
> > The assert at least looks like it is going to triggert at some point,
> > but, would it be a problem if the sizes to not match?
> >
>
> I really can't remember what exactly happened but I do remember it did
> lead to a bug (it's been already part of the chck-in of new SRA so svn
> history does not help). We copy access tree children accross
> assignments and also change the type of the LHS access to a scalar if
> the RHS access is a scalar (assignments into a structure containing
> just one scalar) and both could lead to some access tree children
> covering larger part of the aggregate than the parent, making the
> children un-findable or even creating overlaps which are prohibited
> for SRA candidates.
>
> But as I wrote before, I'll be happy to just remove the FIXME comment.
I'd just remove the comment then.
Richard.
> Martin
>
>
> > Richard.
> >
> >
> > > 2011-01-06 Martin Jambor <mjambor@suse.cz>
> > >
> > > * tree-sra.c (build_accesses_from_assign): Make size equality test
> > > an assert.
> > >
> > > Index: src/gcc/tree-sra.c
> > > ===================================================================
> > > --- src.orig/gcc/tree-sra.c
> > > +++ src/gcc/tree-sra.c
> > > @@ -1175,13 +1175,11 @@ build_accesses_from_assign (gimple stmt)
> > > && !lacc->grp_unscalarizable_region
> > > && !racc->grp_unscalarizable_region
> > > && AGGREGATE_TYPE_P (TREE_TYPE (lhs))
> > > - /* FIXME: Turn the following line into an assert after PR 40058 is
> > > - fixed. */
> > > - && lacc->size == racc->size
> > > && useless_type_conversion_p (lacc->type, racc->type))
> > > {
> > > struct assign_link *link;
> > >
> > > + gcc_assert (lacc->size == racc->size);
> > > link = (struct assign_link *) pool_alloc (link_pool);
> > > memset (link, 0, sizeof (struct assign_link));
>
>
--
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer