[PATCH] Replace a SRA FIXME with an assert

[PATCH] Replace a SRA FIXME with an assert

[Martin Jambor]

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?

Thanks,

Martin


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));
 

Re: [PATCH] Replace a SRA FIXME with an assert

[Richard Guenther]

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.

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?

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));

Re: [PATCH] Replace a SRA FIXME with an assert

[Martin Jambor]

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).

> 
> 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.

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));

Re: [PATCH] Replace a SRA FIXME with an assert

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3627 bytes --]

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

Re: [PATCH] Replace a SRA FIXME with an assert

[Martin Jambor]

On Wed, Mar 21, 2012 at 08:46:49AM +0100, Richard Guenther wrote:
> 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.
> 

OK, I committed the following (after including it in a bootstrap of
another patch)

Thanks,

Martin


2012-03-23  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (build_accesses_from_assign): Remove FIXME comment.

Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -1175,8 +1175,6 @@ 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))
     {