[lto][patch] remove local variables from types (after gimplification)

[lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

Richard pointed out that changing a pointer to VLA into a normal
pointer would introduce performance regressions on the lto branch.

Since the memref branch is going to clean things up on this area and
we have to move forward on lto, Diego and I agreed it would be better
to just patch the types before LTO.

Is the attached patch OK for LTO? I know there will be other places
that expect these fields to be non-NULL, but we can fix them as they
are hit.

Bootstraping and regression testing it now.

2008-06-23  Rafael Espindola  <espindola@google.com>

	* calls.c (must_pass_in_stack_var_size): handle a NULL TYPE_SIZE.
	* expr.c (store_field): handle a NULL TYPE_SIZE.
	* tree.c (reset_type_lang_specific): set TYPE_SIZE_UNIT,
	TYPE_SIZE and TYPE_MAX_VALUE to NULL_TREE if they are not INTEGER_CST.

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

[-- Attachment #2: vla.patch --]
[-- Type: application/octet-stream, Size: 1861 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index e7799f0..022bdaf 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -4301,11 +4301,13 @@ must_pass_in_stack_var_size (enum machine_mode mode ATTRIBUTE_UNUSED,
 bool
 must_pass_in_stack_var_size_or_pad (enum machine_mode mode, const_tree type)
 {
+  tree size;
   if (!type)
     return false;
 
+  size = TYPE_SIZE (type);
   /* If the type has variable size...  */
-  if (TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
+  if (!size || TREE_CODE (size) != INTEGER_CST)
     return true;
 
   /* If the type is marked as addressable (it is required
diff --git a/gcc/expr.c b/gcc/expr.c
index e2693d8..6898530 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -5752,6 +5752,7 @@ store_field (rtx target, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
 	 RHS isn't the same size as the bitfield, we must use bitfield
 	 operations.  */
       || (bitsize >= 0
+	  && TYPE_SIZE (TREE_TYPE (exp))
 	  && TREE_CODE (TYPE_SIZE (TREE_TYPE (exp))) == INTEGER_CST
 	  && compare_tree_int (TYPE_SIZE (TREE_TYPE (exp)), bitsize) != 0))
     {
diff --git a/gcc/tree.c b/gcc/tree.c
index dbf657b..79bff67 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -3783,6 +3783,26 @@ reset_type_lang_specific (void **slot, void *unused ATTRIBUTE_UNUSED)
 {
   tree decl = *(tree*)slot;
   lang_hooks.reset_lang_specifics (decl);
+
+  if (TREE_CODE (decl) == ARRAY_TYPE)
+    {
+      tree unit_size = TYPE_SIZE_UNIT (decl);
+      tree size = TYPE_SIZE (decl);
+
+      if (unit_size && TREE_CODE (unit_size) != INTEGER_CST)
+	TYPE_SIZE_UNIT (decl) = NULL_TREE;
+
+      if (size && TREE_CODE (size) != INTEGER_CST)
+	TYPE_SIZE (decl) = NULL_TREE;
+    }
+
+  if (TREE_CODE (decl) == INTEGER_TYPE)
+    {
+      tree max = TYPE_MAX_VALUE (decl);
+      if (max && TREE_CODE (max) != INTEGER_CST)
+	TYPE_MAX_VALUE (decl) = NULL_TREE;
+    }
+
   return 1;
 }
 

Re: [lto][patch] remove local variables from types (after gimplification)

[Diego Novillo]

On Mon, Jun 23, 2008 at 10:43, Rafael Espindola <espindola@google.com> wrote:

> 2008-06-23  Rafael Espindola  <espindola@google.com>
>
>        * calls.c (must_pass_in_stack_var_size): handle a NULL TYPE_SIZE.
>        * expr.c (store_field): handle a NULL TYPE_SIZE.
>        * tree.c (reset_type_lang_specific): set TYPE_SIZE_UNIT,
>        TYPE_SIZE and TYPE_MAX_VALUE to NULL_TREE if they are not INTEGER_CST.

OK.


Diego.

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Mon, Jun 23, 2008 at 4:43 PM, Rafael Espindola <espindola@google.com> wrote:
> Richard pointed out that changing a pointer to VLA into a normal
> pointer would introduce performance regressions on the lto branch.
>
> Since the memref branch is going to clean things up on this area and
> we have to move forward on lto, Diego and I agreed it would be better
> to just patch the types before LTO.
>
> Is the attached patch OK for LTO? I know there will be other places
> that expect these fields to be non-NULL, but we can fix them as they
> are hit.
>
> Bootstraping and regression testing it now.

This works for me.  Note that if either TYPE_SIZE_UNIT or
TYPE_SIZE is NULL, both should be.  Also the same handling
of TYPE_MAX_VALUE should apply to TYPE_MIN_VALUE - and
rather than setting them to zero I would suggest to set them to
the types natural bounds based on its precision.  And it looks like
you miss TYPE_DOMAIN of ARRAY_TYPE, which contains
the variable bounds of an array.

Richard.

> 2008-06-23  Rafael Espindola  <espindola@google.com>
>
>        * calls.c (must_pass_in_stack_var_size): handle a NULL TYPE_SIZE.
>        * expr.c (store_field): handle a NULL TYPE_SIZE.
>        * tree.c (reset_type_lang_specific): set TYPE_SIZE_UNIT,
>        TYPE_SIZE and TYPE_MAX_VALUE to NULL_TREE if they are not INTEGER_CST.
>
> Cheers,
> --
> Rafael Avila de Espindola
>
> Google Ireland Ltd.
> Gordon House
> Barrow Street
> Dublin 4
> Ireland
>
> Registered in Dublin, Ireland
> Registration Number: 368047
>

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

2008/6/24 Richard Guenther <richard.guenther@gmail.com>:
> On Mon, Jun 23, 2008 at 4:43 PM, Rafael Espindola <espindola@google.com> wrote:
> This works for me.  Note that if either TYPE_SIZE_UNIT or
> TYPE_SIZE is NULL, both should be.
Will try to add that.

> Also the same handling
> of TYPE_MAX_VALUE should apply to TYPE_MIN_VALUE - and
> rather than setting them to zero I would suggest to set them to
> the types natural bounds based on its precision.
Any objections to have that semantics with NULL? Will try to add it to
TYPE_MIN_VALUE.

>  And it looks like
> you miss TYPE_DOMAIN of ARRAY_TYPE, which contains
> the variable bounds of an array.

The callback gets passed all types. In particular, it gets passed the
TYPE_DOMAIN and we clean up the TYPE_MAX_VALUE of it. I did try
setting the TYPE_DOMAIN to NULL but found many places that assume it
is not NULL.

> Richard.

Thanks,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Tue, Jun 24, 2008 at 3:04 PM, Rafael Espindola <espindola@google.com> wrote:
> 2008/6/24 Richard Guenther <richard.guenther@gmail.com>:
>> On Mon, Jun 23, 2008 at 4:43 PM, Rafael Espindola <espindola@google.com> wrote:
>> This works for me.  Note that if either TYPE_SIZE_UNIT or
>> TYPE_SIZE is NULL, both should be.
> Will try to add that.
>
>> Also the same handling
>> of TYPE_MAX_VALUE should apply to TYPE_MIN_VALUE - and
>> rather than setting them to zero I would suggest to set them to
>> the types natural bounds based on its precision.
> Any objections to have that semantics with NULL? Will try to add it to
> TYPE_MIN_VALUE.

While we in most places check for NULL TYPE_MIN/MAX_VALUE we
should not need to - so I'd like you to adjust it instead of NULLing it.

>>  And it looks like
>> you miss TYPE_DOMAIN of ARRAY_TYPE, which contains
>> the variable bounds of an array.
>
> The callback gets passed all types. In particular, it gets passed the
> TYPE_DOMAIN and we clean up the TYPE_MAX_VALUE of it. I did try
> setting the TYPE_DOMAIN to NULL but found many places that assume it
> is not NULL.

I think NULL TYPE_DOMAIN is/was used for incomplete arrays like
int a[] (but this may have changed to a NULL TYPE_SIZE).

Richard.

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

> While we in most places check for NULL TYPE_MIN/MAX_VALUE we
> should not need to - so I'd like you to adjust it instead of NULLing it.

OK. I think I got it right. Can you take a look at the attached patch?
It would probably be cleaner to split
set_min_and_max_values_for_integral_type, but I would like to reduce
the difference from branch to trunk.

Just calling set_min_and_max_values_for_integral_type unconditionally
doesn't work because set_sizetype tries to be smart and copies the
type cache from type A to type B and when we try to build a constant
for type B, build_int_cst_wide aborts.

2008-06-24  Rafael Espindola  <espindola@google.com>

	* calls.c (must_pass_in_stack_var_size_or_pad): Don't check if
	TYPE_SIZE is NULL.
	* expr.c (store_field): Don't check if TYPE_SIZE is NULL.
	* tree.c (reset_type_lang_specific): Set both TYPE_SIZE_UNIT
	and TYPE_SIZE or none of them. Don't set TYPE_MAX_VALUE to
	NULL. call set_min_and_max_values_for_integral_type.

> Richard.
>

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

[-- Attachment #2: types.patch --]
[-- Type: application/octet-stream, Size: 2444 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 022bdaf..e7799f0 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -4301,13 +4301,11 @@ must_pass_in_stack_var_size (enum machine_mode mode ATTRIBUTE_UNUSED,
 bool
 must_pass_in_stack_var_size_or_pad (enum machine_mode mode, const_tree type)
 {
-  tree size;
   if (!type)
     return false;
 
-  size = TYPE_SIZE (type);
   /* If the type has variable size...  */
-  if (!size || TREE_CODE (size) != INTEGER_CST)
+  if (TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
     return true;
 
   /* If the type is marked as addressable (it is required
diff --git a/gcc/expr.c b/gcc/expr.c
index 6898530..e2693d8 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -5752,7 +5752,6 @@ store_field (rtx target, HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos,
 	 RHS isn't the same size as the bitfield, we must use bitfield
 	 operations.  */
       || (bitsize >= 0
-	  && TYPE_SIZE (TREE_TYPE (exp))
 	  && TREE_CODE (TYPE_SIZE (TREE_TYPE (exp))) == INTEGER_CST
 	  && compare_tree_int (TYPE_SIZE (TREE_TYPE (exp)), bitsize) != 0))
     {
diff --git a/gcc/tree.c b/gcc/tree.c
index 79bff67..f69c593 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -3789,18 +3789,29 @@ reset_type_lang_specific (void **slot, void *unused ATTRIBUTE_UNUSED)
       tree unit_size = TYPE_SIZE_UNIT (decl);
       tree size = TYPE_SIZE (decl);
 
-      if (unit_size && TREE_CODE (unit_size) != INTEGER_CST)
-	TYPE_SIZE_UNIT (decl) = NULL_TREE;
-
-      if (size && TREE_CODE (size) != INTEGER_CST)
-	TYPE_SIZE (decl) = NULL_TREE;
+      if ((unit_size && TREE_CODE (unit_size) != INTEGER_CST)
+	  || (size && TREE_CODE (size) != INTEGER_CST))
+	{
+	  TYPE_SIZE_UNIT (decl) = NULL_TREE;
+	  TYPE_SIZE (decl) = NULL_TREE;
+	}
     }
 
   if (TREE_CODE (decl) == INTEGER_TYPE)
     {
-      tree max = TYPE_MAX_VALUE (decl);
-      if (max && TREE_CODE (max) != INTEGER_CST)
-	TYPE_MAX_VALUE (decl) = NULL_TREE;
+      tree old_max = TYPE_MAX_VALUE (decl);
+      tree old_min = TYPE_MIN_VALUE (decl);
+
+      if ((old_max && TREE_CODE (old_max) != INTEGER_CST)
+	  || (old_min && TREE_CODE (old_min) != INTEGER_CST))
+	  set_min_and_max_values_for_integral_type (decl,
+						    TYPE_PRECISION (decl),
+				 		    TYPE_UNSIGNED (decl));
+
+      if (old_max && TREE_CODE (old_max) == INTEGER_CST)
+	TYPE_MAX_VALUE (decl) = old_max;
+      if (old_min && TREE_CODE (old_min) == INTEGER_CST)
+	TYPE_MIN_VALUE (decl) = old_min;
     }
 
   return 1;

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Tue, Jun 24, 2008 at 10:26 PM, Rafael Espindola <espindola@google.com> wrote:
>> While we in most places check for NULL TYPE_MIN/MAX_VALUE we
>> should not need to - so I'd like you to adjust it instead of NULLing it.
>
> OK. I think I got it right. Can you take a look at the attached patch?
> It would probably be cleaner to split
> set_min_and_max_values_for_integral_type, but I would like to reduce
> the difference from branch to trunk.
>
> Just calling set_min_and_max_values_for_integral_type unconditionally
> doesn't work because set_sizetype tries to be smart and copies the
> type cache from type A to type B and when we try to build a constant
> for type B, build_int_cst_wide aborts.

Looks good.  Did you figure out the expr.c and calls.c hunks were not needed?

Thanks,
Richard.

> 2008-06-24  Rafael Espindola  <espindola@google.com>
>
>        * calls.c (must_pass_in_stack_var_size_or_pad): Don't check if
>        TYPE_SIZE is NULL.
>        * expr.c (store_field): Don't check if TYPE_SIZE is NULL.
>        * tree.c (reset_type_lang_specific): Set both TYPE_SIZE_UNIT
>        and TYPE_SIZE or none of them. Don't set TYPE_MAX_VALUE to
>        NULL. call set_min_and_max_values_for_integral_type.
>
>> Richard.
>>
>
> Cheers,
> --
> Rafael Avila de Espindola
>
> Google Ireland Ltd.
> Gordon House
> Barrow Street
> Dublin 4
> Ireland
>
> Registered in Dublin, Ireland
> Registration Number: 368047
>

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

> Looks good.  Did you figure out the expr.c and calls.c hunks were not needed?

I added them in my previous patch :-)

> Thanks,
> Richard.

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Tue, Jun 24, 2008 at 10:44 PM, Rafael Espindola <espindola@google.com> wrote:
>> Looks good.  Did you figure out the expr.c and calls.c hunks were not needed?
>
> I added them in my previous patch :-)

Yes, but they made sense to me, which is why I ask ;)

Richard.

Re: [lto][patch] remove local variables from types (after gimplification)

[Bill Maddox]

On Tue, Jun 24, 2008 at 1:26 PM, Rafael Espindola <espindola@google.com> wrote:
> 2008-06-24  Rafael Espindola  <espindola@google.com>
>
>        * calls.c (must_pass_in_stack_var_size_or_pad): Don't check if
>        TYPE_SIZE is NULL.
>        * expr.c (store_field): Don't check if TYPE_SIZE is NULL.
>        * tree.c (reset_type_lang_specific): Set both TYPE_SIZE_UNIT
>        and TYPE_SIZE or none of them. Don't set TYPE_MAX_VALUE to
>        NULL. call set_min_and_max_values_for_integral_type.

I updated to top of lto branch, and noticed the following test was
first to fail:

   FAIL: gcc.c-torture/compile/20020210-1.c  -O2 -flto  (internal
compiler error)

In this case, the failure is due to an attempt to stream out a SAVE_EXPR node.
I have a development sandbox that includes the patch, but is apparently choosing
a different order in which to stream out the nodes, and which exhibits a much
more interesting failure, encountering a local variable in a type.
The program is
trivial:

  void f(int a, struct {int b[a];} c) {}

The problem is that the size of the *structure* type is variable, due
to the embedded
VLA.  While this program is not valid C99, it is apparently permitted
as a GNU extension,
and similar constructs arise in other languages, e.g. Ada.  Clearly we
are not catching
all of the occurrences of variables within types.  This may just be an
oversight, but I'd
like to know if it is generally the case that the middle-end/back-end
does not not need
this information, or whether the justification for erasing the size of
array types is specific
to the case of arrays.

--Bill

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

> I updated to top of lto branch, and noticed the following test was
> first to fail:
>
>   FAIL: gcc.c-torture/compile/20020210-1.c  -O2 -flto  (internal
> compiler error)
>
> In this case, the failure is due to an attempt to stream out a SAVE_EXPR node.
> I have a development sandbox that includes the patch, but is apparently choosing
> a different order in which to stream out the nodes, and which exhibits a much
> more interesting failure, encountering a local variable in a type.
> The program is
> trivial:
>
>  void f(int a, struct {int b[a];} c) {}
>
> The problem is that the size of the *structure* type is variable, due
> to the embedded
> VLA.  While this program is not valid C99, it is apparently permitted
> as a GNU extension,
> and similar constructs arise in other languages, e.g. Ada.  Clearly we
> are not catching
> all of the occurrences of variables within types.  This may just be an
> oversight, but I'd
> like to know if it is generally the case that the middle-end/back-end
> does not not need
> this information, or whether the justification for erasing the size of
> array types is specific
> to the case of arrays.

I am sure it doesn't need to use it. I am not sure if it is using it.

Thanks for the test. I will try to remove this variable tomorrow.

> --Bill
>


Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Wed, Jun 25, 2008 at 11:13 PM, Rafael Espindola <espindola@google.com> wrote:
>> I updated to top of lto branch, and noticed the following test was
>> first to fail:
>>
>>   FAIL: gcc.c-torture/compile/20020210-1.c  -O2 -flto  (internal
>> compiler error)
>>
>> In this case, the failure is due to an attempt to stream out a SAVE_EXPR node.
>> I have a development sandbox that includes the patch, but is apparently choosing
>> a different order in which to stream out the nodes, and which exhibits a much
>> more interesting failure, encountering a local variable in a type.
>> The program is
>> trivial:
>>
>>  void f(int a, struct {int b[a];} c) {}
>>
>> The problem is that the size of the *structure* type is variable, due
>> to the embedded
>> VLA.  While this program is not valid C99, it is apparently permitted
>> as a GNU extension,
>> and similar constructs arise in other languages, e.g. Ada.  Clearly we
>> are not catching
>> all of the occurrences of variables within types.  This may just be an
>> oversight, but I'd
>> like to know if it is generally the case that the middle-end/back-end
>> does not not need
>> this information, or whether the justification for erasing the size of
>> array types is specific
>> to the case of arrays.
>
> I am sure it doesn't need to use it. I am not sure if it is using it.
>
> Thanks for the test. I will try to remove this variable tomorrow.

If it needs it (currently this for example happens for structure
assignments), the
variable is wrapped in a WITH_SIZE_EXPR node which has the size as
and explicit operand (this is created at gimplification time).

Richard.

Re: [lto][patch] remove local variables from types (after gimplification)

[Bill Maddox]

The following test segfaults, in tree.c (integer_onep):

   FAIL: gcc.c-torture/compile/20071108-1.c  -O2 -flto  (internal
compiler error)

The problem is a call with a NULL argument on line 519 of tree-ssa-forwprop.c:

514       tree index;
515
516       /* Try to find an expression for a proper index.  This is either
517          a multiplication expression by the element size or just the
518          ssa name we came along in case the element size is one.  */
519       if (integer_onep (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (def_rhs)))))
520         index = offset;
521       else
522         {
523           /* Get the offset's defining statement.  */

It looks like we lose an optimization if we can't recognize a constant size.

--Bill

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Thu, Jun 26, 2008 at 1:28 AM, Bill Maddox <maddox@google.com> wrote:
> The following test segfaults, in tree.c (integer_onep):
>
>   FAIL: gcc.c-torture/compile/20071108-1.c  -O2 -flto  (internal
> compiler error)
>
> The problem is a call with a NULL argument on line 519 of tree-ssa-forwprop.c:
>
> 514       tree index;
> 515
> 516       /* Try to find an expression for a proper index.  This is either
> 517          a multiplication expression by the element size or just the
> 518          ssa name we came along in case the element size is one.  */
> 519       if (integer_onep (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (def_rhs)))))
> 520         index = offset;
> 521       else
> 522         {
> 523           /* Get the offset's defining statement.  */
>
> It looks like we lose an optimization if we can't recognize a constant size.

Sure we will.  But we are not talking about NULL-ing non-constant sizes,
no?  Of course TYPE_SIZE{_UNIT} accessors might need a non-NULL
check in some places.

Richard.

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

> I am sure it doesn't need to use it. I am not sure if it is using it.
>
> Thanks for the test. I will try to remove this variable tomorrow.

The attached patch fixes this problem. OK if bootstraps and tests are OK?

2008-05-28  Rafael Espindola  <espindola@google.com>

	* tree.c (reset_type_lang_specific): Set non integer sizes in
	RECORD_TYPEs to NULL_TREE.

>> --Bill


Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

[-- Attachment #2: struct-size.patch --]
[-- Type: application/octet-stream, Size: 433 bytes --]

diff --git a/gcc/tree.c b/gcc/tree.c
index f69c593..46600e2 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -3814,6 +3814,13 @@ reset_type_lang_specific (void **slot, void *unused ATTRIBUTE_UNUSED)
 	TYPE_MIN_VALUE (decl) = old_min;
     }
 
+  if (TREE_CODE (decl) == RECORD_TYPE)
+    {
+      tree size = TYPE_SIZE (decl);
+      if (size && TREE_CODE (size) != INTEGER_CST)
+	TYPE_SIZE (decl) = NULL_TREE;
+    }
+
   return 1;
 }
 

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

>> It looks like we lose an optimization if we can't recognize a constant size.
>
> Sure we will.  But we are not talking about NULL-ing non-constant sizes,
> no?  Of course TYPE_SIZE{_UNIT} accessors might need a non-NULL
> check in some places.

We are removing only non integer ones. The original idea was to use a
pointer to element type instead of an array, but you convinced us not
to :-)

> Richard.
>


Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Richard Guenther]

On Thu, Jun 26, 2008 at 4:25 PM, Rafael Espindola <espindola@google.com> wrote:
>>> It looks like we lose an optimization if we can't recognize a constant size.
>>
>> Sure we will.  But we are not talking about NULL-ing non-constant sizes,
>> no?  Of course TYPE_SIZE{_UNIT} accessors might need a non-NULL
>> check in some places.
>
> We are removing only non integer ones. The original idea was to use a
> pointer to element type instead of an array, but you convinced us not
> to :-)

Yep ;)  In which case we won't loose optimization opportunities, because
obviously in the cited example the size was _not_ constant (otherwise
TYPE_SIZE wouldn't be zero).

Richard.

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

2008/6/25 Bill Maddox <maddox@google.com>:
> The following test segfaults, in tree.c (integer_onep):
>
>   FAIL: gcc.c-torture/compile/20071108-1.c  -O2 -flto  (internal
> compiler error)

The attached patch fixes the problem. OK if bootstrap and tests are OK?

2008-06-26  Rafael Espindola  <espindola@google.com>

	* tree-ssa-forwprop.c
	(forward_propagate_addr_into_variable_array_index): Check if
	the size of an array is NULL.

is this the right way to format the changelog with ultra long
function names? :-)

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

[-- Attachment #2: fix.patch --]
[-- Type: application/octet-stream, Size: 1189 bytes --]

diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c
index cf6cf04..903a4f5 100644
--- a/gcc/tree-ssa-forwprop.c
+++ b/gcc/tree-ssa-forwprop.c
@@ -512,11 +512,14 @@ forward_propagate_addr_into_variable_array_index (tree offset,
 						  tree def_rhs, tree use_stmt)
 {
   tree index;
+  tree pointer_type = TREE_TYPE (def_rhs);
+  tree array_type = TREE_TYPE (pointer_type);
+  tree unit_size = TYPE_SIZE_UNIT (array_type);
 
   /* Try to find an expression for a proper index.  This is either
      a multiplication expression by the element size or just the
      ssa name we came along in case the element size is one.  */
-  if (integer_onep (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (def_rhs)))))
+  if (unit_size && integer_onep (unit_size))
     index = offset;
   else
     {
@@ -536,7 +539,7 @@ forward_propagate_addr_into_variable_array_index (tree offset,
       if (TREE_CODE (offset) != MULT_EXPR
 	  || TREE_CODE (TREE_OPERAND (offset, 1)) != INTEGER_CST
 	  || !simple_cst_equal (TREE_OPERAND (offset, 1),
-				TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (def_rhs)))))
+				unit_size))
 	return false;
 
       /* The first operand to the MULT_EXPR is the desired index.  */

Re: [lto][patch] remove local variables from types (after gimplification)

[Diego Novillo]

On Thu, Jun 26, 2008 at 12:08, Rafael Espindola <espindola@google.com> wrote:

> 2008-06-26  Rafael Espindola  <espindola@google.com>
>
>        * tree-ssa-forwprop.c
>        (forward_propagate_addr_into_variable_array_index): Check if
>        the size of an array is NULL.

OK.

> is this the right way to format the changelog with ultra long
> function names? :-)

Well, you could put the function name in the previous line, unless it
wraps 80cols.  In which case, there really isn't an elegant way of
dealing with this.


Diego.

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

> The attached patch fixes this problem. OK if bootstraps and tests are OK?
Tested something wrong.

I have a patch that fixes the type specific parts. I will do a merge
form trunk to get the decl -> tree mapping. With it I can fix the
rest.

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Rafael Espindola]

>> 2008-06-26  Rafael Espindola  <espindola@google.com>
>>
>>        * tree-ssa-forwprop.c
>>        (forward_propagate_addr_into_variable_array_index): Check if
>>        the size of an array is NULL.
>
> OK.

All tests are OK. Just waiting for gcc.gnu.org to get back online.

> Diego.
>

Cheers,
-- 
Rafael Avila de Espindola

Google Ireland Ltd.
Gordon House
Barrow Street
Dublin 4
Ireland

Registered in Dublin, Ireland
Registration Number: 368047

Re: [lto][patch] remove local variables from types (after gimplification)

[Bill Maddox]

I am seeing another regression failure due to VLAs:

Executing on host: /usr/local/google/maddox/lto-baseline/obj/gcc/xgcc
-B/usr/local/google/maddox/lto-baseline/obj/gcc/
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c
 -w  -O2 -flto  -fno-show-column  -lm   -o
/usr/local/google/maddox/lto-baseline/obj/gcc/testsuite/gcc/20040308-1.x2
   (timeout = 300)
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c:21:
internal compiler error: in output_tree, at lto-function-out.c:3149

The problem is that  a FIELD_DECL  has a variable as it s offset:

Breakpoint 1, internal_error (gmsgid=0x88fcc64 "output_tree") at
../../gcc/gcc/diagnostic.c:598
598       va_start (ap, gmsgid);
(gdb) bt
#0  internal_error (gmsgid=0x88fcc64 "output_tree") at
../../gcc/gcc/diagnostic.c:598
#1  0x08163a79 in fancy_abort (file=0x88fcc64 "output_tree",
line=3149, function=0x88fcc64 "output_tree") at
../../gcc/gcc/diagnostic.c:654
#2  0x08766727 in output_tree (ob=0x8a03660, expr=0xf7f01398) at
../../gcc/gcc/lto-function-out.c:3149
#3  0x087633e9 in output_field_decl (ob=0x8a03660, decl=0xf7f0105c) at
../../gcc/gcc/lto-function-out.c:2499
#4  0x0876666b in output_tree (ob=0x8a03660, expr=0xf7f0105c) at
../../gcc/gcc/lto-function-out.c:3137
#5  0x08763467 in output_field_decl (ob=0x8a03660, decl=0xf7f01000) at
../../gcc/gcc/lto-function-out.c:2510
#6  0x0876666b in output_tree (ob=0x8a03660, expr=0xf7f01000) at
../../gcc/gcc/lto-function-out.c:3137
#7  0x087648a1 in output_type (ob=0x8a03660, type=0xf7ef4798,
tag=LTO_record_type) at ../../gcc/gcc/lto-function-out.c:2901
#8  0x0876a227 in output_tree (ob=0x8a03660, expr=0xf7ef4798) at
../../gcc/gcc/lto-function-out.c:3418
#9  0x0876331f in output_field_decl (ob=0x8a03660, decl=0xf7f010b8) at
../../gcc/gcc/lto-function-out.c:2485
#10 0x0876666b in output_tree (ob=0x8a03660, expr=0xf7f010b8) at
../../gcc/gcc/lto-function-out.c:3137
#11 0x0876bfa1 in write_global_stream (ob=0x8a03660, v=0x8a05710) at
../../gcc/gcc/lto-section-out.c:786
#12 0x0876c254 in produce_asm_for_decls () at
../../gcc/gcc/lto-section-out.c:866
#13 0x082cbee2 in ipa_write_summaries_1 (pass=0x898dc40) at
../../gcc/gcc/passes.c:1360
#14 0x082cc168 in execute_ipa_pass_list (pass=0x898dbe0) at
../../gcc/gcc/passes.c:1419
#15 0x08594596 in cgraph_optimize () at ../../gcc/gcc/cgraphunit.c:1351
#16 0x0805fa94 in c_write_global_declarations () at ../../gcc/gcc/c-decl.c:8022
#17 0x0834d6e7 in toplev_main (argc=143641700, argv=0xffffd694) at
../../gcc/gcc/toplev.c:983
#18 0x080dd58a in main ()
(gdb) up
#1  0x08163a79 in fancy_abort (file=0x88fcc64 "output_tree",
line=3149, function=0x88fcc64 "output_tree") at
../../gcc/gcc/diagnostic.c:654
654       internal_error ("in %s, at %s:%d", function, trim_filename
(file), line);
(gdb) up
#2  0x08766727 in output_tree (ob=0x8a03660, expr=0xf7f01398) at
../../gcc/gcc/lto-function-out.c:3149
3149            gcc_unreachable ();
(gdb) list
3144        case VAR_DECL:
3145          if (TREE_STATIC (expr) || DECL_EXTERNAL (expr))
3146            output_var_decl (ob, expr);
3147          else
3148            /* We should not be seeing local variables here.  */
3149            gcc_unreachable ();
3150          break;
3151
3152        case PARM_DECL:
3153          output_parm_decl (ob, expr);
(gdb) pt expr
 <var_decl 0xf7f01398 D.1188
    type <integer_type 0xf7e8d000 unsigned int public unsigned sizetype SI
        size <integer_cst 0xf7e816ac constant 32>
        unit size <integer_cst 0xf7e81498 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0xf7e930d8
precision 32 min <integer_cst 0xf7e816c8 0> max <integer_cst
0xf7e81cb0 -1>>
    used unsigned ignored SI file
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c
line 9 col 3 size <integer_cst 0xf7e816ac 32> unit size <integer_cst
0xf7e81498 4>
    align 32 context <function_decl 0xf7ef3b80 foo> chain <var_decl
0xf7f013f4 D.1189>>
(gdb) up
#3  0x087633e9 in output_field_decl (ob=0x8a03660, decl=0xf7f0105c) at
../../gcc/gcc/lto-function-out.c:2499
2499      output_tree (ob, decl->field_decl.offset);
(gdb) pt decl
 <field_decl 0xf7f0105c b
    type <integer_type 0xf7ef4948 sizes-gimplified public unsigned QI
        size <integer_cst 0xf7e81524 constant 8>
        unit size <integer_cst 0xf7e81540 constant 1>
        align 8 symtab 0 alias set -1 canonical type 0xf7ef4948
precision 1 min <integer_cst 0xf7f00b28 0> max <integer_cst 0xf7f00b44
1>>
    unsigned external packed bit-field nonaddressable decl_4 QI file
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c
line 7 col 18
    size <integer_cst 0xf7e81d3c type <integer_type 0xf7e8d06c
bit_size_type> constant 1> unit size <integer_cst 0xf7e81540 1>
    align 1 offset_align 8
    offset <var_decl 0xf7f01398 D.1188
        type <integer_type 0xf7e8d000 unsigned int public unsigned sizetype SI
            size <integer_cst 0xf7e816ac constant 32>
            unit size <integer_cst 0xf7e81498 constant 4>
            align 32 symtab 0 alias set -1 canonical type 0xf7e930d8
precision 32 min <integer_cst 0xf7e816c8 0> max <integer_cst
0xf7e81cb0 -1>>
        used unsigned ignored SI file
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c
line 9 col 3 size <integer_cst 0xf7e816ac 32> unit size <integer_cst
0xf7e81498 4>
        align 32 context <function_decl 0xf7ef3b80 foo>
        chain <var_decl 0xf7f013f4 D.1189 type <integer_type
0xf7e8d000 unsigned int>
            used unsigned ignored SI file
/usr/local/google/maddox/lto-baseline/gcc/gcc/testsuite/gcc.c-torture/execute/20040308-1.c
line 9 col 3 size <integer_cst 0xf7e816ac 32> unit size <integer_cst
0xf7e81498 4>
            align 32 context <function_decl 0xf7ef3b80 foo> chain
<var_decl 0xf7f01450 D.1190>>>
    bit offset <integer_cst 0xf7e81c24 type <integer_type 0xf7e8d06c
bit_size_type> constant 0>
    bit_field_type <integer_type 0xf7e8d360 unsigned int public
unsigned SI size <integer_cst 0xf7e816ac 32> unit size <integer_cst
0xf7e81498 4>
        align 32 symtab 0 alias set -1 canonical type 0xf7e8d360
precision 32 min <integer_cst 0xf7e816c8 0> max <integer_cst
0xf7e81690 4294967295>
        pointer_to_this <pointer_type 0xf7e9772c>> context
<record_type 0xf7ef4798 S> chain <field_decl 0xf7f010b8 i2>>
(gdb) ptu decl->field_decl.offset
$NODE =$1 = 0xf7f01398
D.1188

The good news is that this looks like the last remaining VLA issue on
the C regression tests.

--Bill