* [PATCH] Replace a SRA FIXME with an assert
@ 2012-03-20 14:58 Martin Jambor
2012-03-20 15:09 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Martin Jambor @ 2012-03-20 14:58 UTC (permalink / raw)
To: GCC Patches; +Cc: Richard Guenther
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));
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Replace a SRA FIXME with an assert
2012-03-20 14:58 [PATCH] Replace a SRA FIXME with an assert Martin Jambor
@ 2012-03-20 15:09 ` Richard Guenther
2012-03-20 17:38 ` Martin Jambor
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2012-03-20 15:09 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches
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));
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Replace a SRA FIXME with an assert
2012-03-20 15:09 ` Richard Guenther
@ 2012-03-20 17:38 ` Martin Jambor
2012-03-21 7:47 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Martin Jambor @ 2012-03-20 17:38 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
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));
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Replace a SRA FIXME with an assert
2012-03-20 17:38 ` Martin Jambor
@ 2012-03-21 7:47 ` Richard Guenther
2012-03-23 11:47 ` Martin Jambor
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2012-03-21 7:47 UTC (permalink / raw)
To: Martin Jambor; +Cc: GCC Patches
[-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Replace a SRA FIXME with an assert
2012-03-21 7:47 ` Richard Guenther
@ 2012-03-23 11:47 ` Martin Jambor
0 siblings, 0 replies; 5+ messages in thread
From: Martin Jambor @ 2012-03-23 11:47 UTC (permalink / raw)
To: GCC Patches
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))
{
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-03-23 11:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-20 14:58 [PATCH] Replace a SRA FIXME with an assert Martin Jambor
2012-03-20 15:09 ` Richard Guenther
2012-03-20 17:38 ` Martin Jambor
2012-03-21 7:47 ` Richard Guenther
2012-03-23 11:47 ` Martin Jambor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).