diff mbox

Small fix for walking constructors

Message ID 541B2C24.9010703@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt Sept. 18, 2014, 7:01 p.m. UTC 
This fixes an issue on ptx where we fail to output a declaration for a 
variable. The testcase is c-torture/compile/pr34856.c, and the cause of 
the problem is that the variable g is never inserted into the varpool, 
which is where a future patch will look for references to variables not 
defined in the current translation unit (ptx assembly requires 
declarations for these too).

Bootstrapped and tested on x86_64-linux, ok?


Bernd

Comments

Jeff Law Sept. 18, 2014, 8:38 p.m. UTC | #1 
On 09/18/14 13:01, Bernd Schmidt wrote:
> This fixes an issue on ptx where we fail to output a declaration for a
> variable. The testcase is c-torture/compile/pr34856.c, and the cause of
> the problem is that the variable g is never inserted into the varpool,
> which is where a future patch will look for references to variables not
> defined in the current translation unit (ptx assembly requires
> declarations for these too).
>
> Bootstrapped and tested on x86_64-linux, ok?
>
>
> Bernd
>
> walk-more.diff
>
>
> commit 968a508fdd5c413147b9c26d37633bf7ab7a7e65
> Author: Bernd Schmidt<bernds@codesourcery.com>
> Date:   Thu Sep 11 14:35:01 2014 +0200
>
>      Fix handling of CONSTRUCTORs in gimple-walk.
>
>      	* gimple-walk.c (walk_stmt_load_store_addr_ops): Look past casts when
>      	dealing with CONSTRUCTORs.
OK.
Jeff
Richard Biener Sept. 22, 2014, 8:58 a.m. UTC | #2 
On Thu, Sep 18, 2014 at 10:38 PM, Jeff Law <law@redhat.com> wrote:
> On 09/18/14 13:01, Bernd Schmidt wrote:
>>
>> This fixes an issue on ptx where we fail to output a declaration for a
>> variable. The testcase is c-torture/compile/pr34856.c, and the cause of
>> the problem is that the variable g is never inserted into the varpool,
>> which is where a future patch will look for references to variables not
>> defined in the current translation unit (ptx assembly requires
>> declarations for these too).
>>
>> Bootstrapped and tested on x86_64-linux, ok?
>>
>>
>> Bernd
>>
>> walk-more.diff
>>
>>
>> commit 968a508fdd5c413147b9c26d37633bf7ab7a7e65
>> Author: Bernd Schmidt<bernds@codesourcery.com>
>> Date:   Thu Sep 11 14:35:01 2014 +0200
>>
>>      Fix handling of CONSTRUCTORs in gimple-walk.
>>
>>         * gimple-walk.c (walk_stmt_load_store_addr_ops): Look past casts
>> when
>>         dealing with CONSTRUCTORs.
>
> OK.

Errr - certainly not.

It seems to me that walk_stmt_load_store_addr_ops is called on
bogus input.  The function is supposed to be called on GIMPLE
stmts and in GIMPLE stmts CONSTRUCTORs may _not_ have
conversions in their elements.

Please revert if you have applied already.

Thanks,
Richard.

> Jeff
>
Richard Biener Sept. 22, 2014, 9 a.m. UTC | #3 
On Mon, Sep 22, 2014 at 10:58 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Sep 18, 2014 at 10:38 PM, Jeff Law <law@redhat.com> wrote:
>> On 09/18/14 13:01, Bernd Schmidt wrote:
>>>
>>> This fixes an issue on ptx where we fail to output a declaration for a
>>> variable. The testcase is c-torture/compile/pr34856.c, and the cause of
>>> the problem is that the variable g is never inserted into the varpool,
>>> which is where a future patch will look for references to variables not
>>> defined in the current translation unit (ptx assembly requires
>>> declarations for these too).
>>>
>>> Bootstrapped and tested on x86_64-linux, ok?
>>>
>>>
>>> Bernd
>>>
>>> walk-more.diff
>>>
>>>
>>> commit 968a508fdd5c413147b9c26d37633bf7ab7a7e65
>>> Author: Bernd Schmidt<bernds@codesourcery.com>
>>> Date:   Thu Sep 11 14:35:01 2014 +0200
>>>
>>>      Fix handling of CONSTRUCTORs in gimple-walk.
>>>
>>>         * gimple-walk.c (walk_stmt_load_store_addr_ops): Look past casts
>>> when
>>>         dealing with CONSTRUCTORs.
>>
>> OK.
>
> Errr - certainly not.
>
> It seems to me that walk_stmt_load_store_addr_ops is called on
> bogus input.  The function is supposed to be called on GIMPLE
> stmts and in GIMPLE stmts CONSTRUCTORs may _not_ have
> conversions in their elements.
>
> Please revert if you have applied already.

For the testcase I can indeed see


  <bb 2>:
  pin_3 = {(unsigned int) (long int) &g[16]};

but that's invalid GIMPLE, unfortunately not caught by out checker.

Please fix the root cause and add checking to verify_gimple_assign_single.

Thanks,
Richard.



>
> Thanks,
> Richard.
>
>> Jeff
>>
Bernd Schmidt Sept. 22, 2014, 9:52 a.m. UTC | #4 
On 09/22/2014 11:00 AM, Richard Biener wrote:
>> It seems to me that walk_stmt_load_store_addr_ops is called on
>> bogus input.  The function is supposed to be called on GIMPLE
>> stmts and in GIMPLE stmts CONSTRUCTORs may _not_ have
>> conversions in their elements.
>>
>> Please revert if you have applied already.
>
> For the testcase I can indeed see
>
>
>    <bb 2>:
>    pin_3 = {(unsigned int) (long int) &g[16]};
>
> but that's invalid GIMPLE, unfortunately not caught by out checker.
>
> Please fix the root cause and add checking to verify_gimple_assign_single.

Hmm, fix how exactly? What representation do you want for an initializer 
where a pointer is cast to an int (or to a different address space, 
something that will be possible with patches I'll submit in the near 
future)?


Bernd
Richard Biener Sept. 22, 2014, 9:54 a.m. UTC | #5 
On Mon, Sep 22, 2014 at 11:00 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Sep 22, 2014 at 10:58 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Sep 18, 2014 at 10:38 PM, Jeff Law <law@redhat.com> wrote:
>>> On 09/18/14 13:01, Bernd Schmidt wrote:
>>>>
>>>> This fixes an issue on ptx where we fail to output a declaration for a
>>>> variable. The testcase is c-torture/compile/pr34856.c, and the cause of
>>>> the problem is that the variable g is never inserted into the varpool,
>>>> which is where a future patch will look for references to variables not
>>>> defined in the current translation unit (ptx assembly requires
>>>> declarations for these too).
>>>>
>>>> Bootstrapped and tested on x86_64-linux, ok?
>>>>
>>>>
>>>> Bernd
>>>>
>>>> walk-more.diff
>>>>
>>>>
>>>> commit 968a508fdd5c413147b9c26d37633bf7ab7a7e65
>>>> Author: Bernd Schmidt<bernds@codesourcery.com>
>>>> Date:   Thu Sep 11 14:35:01 2014 +0200
>>>>
>>>>      Fix handling of CONSTRUCTORs in gimple-walk.
>>>>
>>>>         * gimple-walk.c (walk_stmt_load_store_addr_ops): Look past casts
>>>> when
>>>>         dealing with CONSTRUCTORs.
>>>
>>> OK.
>>
>> Errr - certainly not.
>>
>> It seems to me that walk_stmt_load_store_addr_ops is called on
>> bogus input.  The function is supposed to be called on GIMPLE
>> stmts and in GIMPLE stmts CONSTRUCTORs may _not_ have
>> conversions in their elements.
>>
>> Please revert if you have applied already.
>
> For the testcase I can indeed see
>
>
>   <bb 2>:
>   pin_3 = {(unsigned int) (long int) &g[16]};
>
> but that's invalid GIMPLE, unfortunately not caught by out checker.
>
> Please fix the root cause and add checking to verify_gimple_assign_single.

I'm on it.  The reason for the invalid GIMPLE is the gimplifier which
says "well, looks like a constant for the target!" and doesn't gimplify
at all then.  Oops (but only for vectors?!).

Introduced by r118747 and only semi-fixed by r129739.  The original
rev. is also just a bugfix for an ICE.

Thus I am testing the following.

2014-09-22  Richard Biener  <rguenther@suse.de>

        * gimplify.c (gimplify_init_constructor): Do not leave
        non-GIMPLE vector constructors around.
        * tree-cfg.c (verify_gimple_assign_single): Verify that
        CONSTRUCTORs have gimple elements.


Richard.






> Thanks,
> Richard.
>
>
>
>>
>> Thanks,
>> Richard.
>>
>>> Jeff
>>>
Richard Biener Sept. 22, 2014, 9:56 a.m. UTC | #6 
On Mon, Sep 22, 2014 at 11:52 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 09/22/2014 11:00 AM, Richard Biener wrote:
>>>
>>> It seems to me that walk_stmt_load_store_addr_ops is called on
>>> bogus input.  The function is supposed to be called on GIMPLE
>>> stmts and in GIMPLE stmts CONSTRUCTORs may _not_ have
>>> conversions in their elements.
>>>
>>> Please revert if you have applied already.
>>
>>
>> For the testcase I can indeed see
>>
>>
>>    <bb 2>:
>>    pin_3 = {(unsigned int) (long int) &g[16]};
>>
>> but that's invalid GIMPLE, unfortunately not caught by out checker.
>>
>> Please fix the root cause and add checking to verify_gimple_assign_single.
>
>
> Hmm, fix how exactly? What representation do you want for an initializer
> where a pointer is cast to an int (or to a different address space,
> something that will be possible with patches I'll submit in the near
> future)?

For the above

  _4 = (long int) &g[16];
  _5 = (unsigned int) _4;
  pin_3 = { _5 };

it's GIMPLE after all, not GENERIC.

Richard.

>
> Bernd
>
diff mbox

Patch

commit 968a508fdd5c413147b9c26d37633bf7ab7a7e65
Author: Bernd Schmidt <bernds@codesourcery.com>
Date:   Thu Sep 11 14:35:01 2014 +0200

    Fix handling of CONSTRUCTORs in gimple-walk.
    
    	* gimple-walk.c (walk_stmt_load_store_addr_ops): Look past casts when
    	dealing with CONSTRUCTORs.

diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
index f4f6757..593f0e2 100644
--- a/gcc/gimple-walk.c
+++ b/gcc/gimple-walk.c
@@ -695,13 +695,17 @@  walk_stmt_load_store_addr_ops (gimple stmt, void *data,
 	      tree val;
 
 	      FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (rhs), ix, val)
-		if (TREE_CODE (val) == ADDR_EXPR)
-		  ret |= visit_addr (stmt, TREE_OPERAND (val, 0), arg, data);
-		else if (TREE_CODE (val) == OBJ_TYPE_REF
-			 && TREE_CODE (OBJ_TYPE_REF_OBJECT (val)) == ADDR_EXPR)
-		  ret |= visit_addr (stmt,
-				     TREE_OPERAND (OBJ_TYPE_REF_OBJECT (val),
-						   0), arg, data);
+		{
+		  while (CONVERT_EXPR_P (val))
+		    val = TREE_OPERAND (val, 0);
+		  if (TREE_CODE (val) == ADDR_EXPR)
+		    ret |= visit_addr (stmt, TREE_OPERAND (val, 0), arg, data);
+		  else if (TREE_CODE (val) == OBJ_TYPE_REF
+			   && TREE_CODE (OBJ_TYPE_REF_OBJECT (val)) == ADDR_EXPR)
+		    ret |= visit_addr (stmt,
+				       TREE_OPERAND (OBJ_TYPE_REF_OBJECT (val),
+						     0), arg, data);
+		}
 	    }
           lhs = gimple_assign_lhs (stmt);
 	  if (TREE_CODE (lhs) == TARGET_MEM_REF