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