Copy assignments for non scalar types

Copy assignments for non scalar types

[Sebastian Pop]

Hi,

While working on the tree-if-conv.c, I realized that the copy
of the contents of a non scalar variable are not correctly done.
The copy assignment triggers this error:

error: virtual SSA name for non-VOP decl
while verifying SSA_NAME _ifc_.2005_46 in statement
# .MEM_394 = VDEF <.MEM_475>
    _ifc_.2005_46 = ops[j_457];

The array reference looks like this:

 <array_ref 0x7ffff4bd65a0
    type <record_type 0x7ffff4ee2c78 simplify_plus_minus_op_data
sizes-gimplified asm_written type_0 BLK

For scalar types, the code that creates the copy is working correctly:

/* Create a new temp variable of type TYPE.  Add GIMPLE_ASSIGN to assign EXP
   to the new variable.  */

static gimple
ifc_temp_var (tree type, tree exp)
{
  const char *name = "_ifc_";
  tree var, new_name;
  gimple stmt;

  /* Create new temporary variable.  */
  var = create_tmp_var (type, name);
  add_referenced_var (var);

  /* Build new statement to assign EXP to new variable.  */
  stmt = gimple_build_assign (var, exp);

  /* Get SSA name for the new variable and set make new statement
     its definition statement.  */
  new_name = make_ssa_name (var, stmt);
  gimple_assign_set_lhs (stmt, new_name);
  SSA_NAME_DEF_STMT (new_name) = stmt;
  update_stmt (stmt);

  return stmt;
}

What is missing in this function to make it handle non scalar types?

Thanks,
Sebastian Pop
--
AMD / Open Source Compiler Engineering / GNU Tools

Re: Copy assignments for non scalar types

[Jakub Jelinek]

On Tue, Apr 13, 2010 at 01:14:11PM -0500, Sebastian Pop wrote:
> /* Create a new temp variable of type TYPE.  Add GIMPLE_ASSIGN to assign EXP
>    to the new variable.  */
> 
> static gimple
> ifc_temp_var (tree type, tree exp)
> {
>   const char *name = "_ifc_";
>   tree var, new_name;
>   gimple stmt;
> 
>   /* Create new temporary variable.  */
>   var = create_tmp_var (type, name);
>   add_referenced_var (var);
> 
>   /* Build new statement to assign EXP to new variable.  */
>   stmt = gimple_build_assign (var, exp);
> 
>   /* Get SSA name for the new variable and set make new statement
>      its definition statement.  */
>   new_name = make_ssa_name (var, stmt);
>   gimple_assign_set_lhs (stmt, new_name);
>   SSA_NAME_DEF_STMT (new_name) = stmt;
>   update_stmt (stmt);
> 
>   return stmt;
> }
> 
> What is missing in this function to make it handle non scalar types?

if (!is_gimple_reg (var))

you shouldn't create SSA name and change the lhs of the stmt.

	Jakub

Re: Copy assignments for non scalar types

[Sebastian Pop]

On Tue, Apr 13, 2010 at 14:42, Jakub Jelinek <jakub@redhat.com> wrote:
> if (!is_gimple_reg (var))
>
> you shouldn't create SSA name and change the lhs of the stmt.
>

This worked well.  Thanks Jakub!

Sebastian

Re: Copy assignments for non scalar types

[Eric Botcazou]

> if (!is_gimple_reg (var))
>
> you shouldn't create SSA name and change the lhs of the stmt.

You might also not be allowed to make a copy in the first place but only to 
take a reference, see gimplify_cond_expr.

-- 
Eric Botcazou

Re: Copy assignments for non scalar types

[Sebastian Pop]

On Tue, Apr 13, 2010 at 13:14, Sebastian Pop <sebpop@gmail.com> wrote:
> Hi,
>
> While working on the tree-if-conv.c, I realized that the copy
> of the contents of a non scalar variable are not correctly done.
> The copy assignment triggers this error:
>
> error: virtual SSA name for non-VOP decl
> while verifying SSA_NAME _ifc_.2005_46 in statement
> # .MEM_394 = VDEF <.MEM_475>
>    _ifc_.2005_46 = ops[j_457];
>
> The array reference looks like this:
>
>  <array_ref 0x7ffff4bd65a0
>    type <record_type 0x7ffff4ee2c78 simplify_plus_minus_op_data
> sizes-gimplified asm_written type_0 BLK
>
> For scalar types, the code that creates the copy is working correctly:
>
> /* Create a new temp variable of type TYPE.  Add GIMPLE_ASSIGN to assign EXP
>   to the new variable.  */
>
> static gimple
> ifc_temp_var (tree type, tree exp)
> {
>  const char *name = "_ifc_";
>  tree var, new_name;
>  gimple stmt;
>
>  /* Create new temporary variable.  */
>  var = create_tmp_var (type, name);
>  add_referenced_var (var);
>
>  /* Build new statement to assign EXP to new variable.  */
>  stmt = gimple_build_assign (var, exp);
>
>  /* Get SSA name for the new variable and set make new statement
>     its definition statement.  */
>  new_name = make_ssa_name (var, stmt);
>  gimple_assign_set_lhs (stmt, new_name);
>  SSA_NAME_DEF_STMT (new_name) = stmt;
>  update_stmt (stmt);
>
>  return stmt;
> }
>
> What is missing in this function to make it handle non scalar types?
>

At least this is missing (but it is not enough, so I am still looking for
a solution):

  if (gimple_referenced_vars (cfun))
    {
      add_referenced_var (t);
      mark_sym_for_renaming (t);
    }

from tree-dfa.c:

/* Build a temporary.  Make sure and register it to be renamed.  */

tree
make_rename_temp (tree type, const char *prefix)
{
  tree t = create_tmp_var (type, prefix);

  if (TREE_CODE (TREE_TYPE (t)) == COMPLEX_TYPE
      || TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
    DECL_GIMPLE_REG_P (t) = 1;

  if (gimple_referenced_vars (cfun))
    {
      add_referenced_var (t);
      mark_sym_for_renaming (t);
    }

  return t;
}

I will replace the call to create_tmp_var with make_rename_temp.

Sebastian

Re: Copy assignments for non scalar types

[Richard Guenther]

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

On Tue, 13 Apr 2010, Sebastian Pop wrote:

> On Tue, Apr 13, 2010 at 13:14, Sebastian Pop <sebpop@gmail.com> wrote:
> > Hi,
> >
> > While working on the tree-if-conv.c, I realized that the copy
> > of the contents of a non scalar variable are not correctly done.
> > The copy assignment triggers this error:
> >
> > error: virtual SSA name for non-VOP decl
> > while verifying SSA_NAME _ifc_.2005_46 in statement
> > # .MEM_394 = VDEF <.MEM_475>
> >    _ifc_.2005_46 = ops[j_457];
> >
> > The array reference looks like this:
> >
> >  <array_ref 0x7ffff4bd65a0
> >    type <record_type 0x7ffff4ee2c78 simplify_plus_minus_op_data
> > sizes-gimplified asm_written type_0 BLK
> >
> > For scalar types, the code that creates the copy is working correctly:
> >
> > /* Create a new temp variable of type TYPE.  Add GIMPLE_ASSIGN to assign EXP
> >   to the new variable.  */
> >
> > static gimple
> > ifc_temp_var (tree type, tree exp)
> > {
> >  const char *name = "_ifc_";
> >  tree var, new_name;
> >  gimple stmt;
> >
> >  /* Create new temporary variable.  */
> >  var = create_tmp_var (type, name);
> >  add_referenced_var (var);
> >
> >  /* Build new statement to assign EXP to new variable.  */
> >  stmt = gimple_build_assign (var, exp);
> >
> >  /* Get SSA name for the new variable and set make new statement
> >     its definition statement.  */
> >  new_name = make_ssa_name (var, stmt);
> >  gimple_assign_set_lhs (stmt, new_name);
> >  SSA_NAME_DEF_STMT (new_name) = stmt;
> >  update_stmt (stmt);
> >
> >  return stmt;
> > }
> >
> > What is missing in this function to make it handle non scalar types?
> >
> 
> At least this is missing (but it is not enough, so I am still looking for
> a solution):
> 
>   if (gimple_referenced_vars (cfun))
>     {
>       add_referenced_var (t);
>       mark_sym_for_renaming (t);
>     }
> 
> from tree-dfa.c:
> 
> /* Build a temporary.  Make sure and register it to be renamed.  */
> 
> tree
> make_rename_temp (tree type, const char *prefix)
> {
>   tree t = create_tmp_var (type, prefix);
> 
>   if (TREE_CODE (TREE_TYPE (t)) == COMPLEX_TYPE
>       || TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
>     DECL_GIMPLE_REG_P (t) = 1;
> 
>   if (gimple_referenced_vars (cfun))
>     {
>       add_referenced_var (t);
>       mark_sym_for_renaming (t);
>     }
> 
>   return t;
> }
> 
> I will replace the call to create_tmp_var with make_rename_temp.

No.  make_rename_temp should go away.  Please.

Btw, you can't make an SSA name of aggregate type.

Richard.

Re: Copy assignments for non scalar types

[Diego Novillo]

On Wed, Apr 14, 2010 at 04:40, Richard Guenther <rguenther@suse.de> wrote:

> No.  make_rename_temp should go away.  Please.

I don't disagree, in principle (less code is always good).  What is
wrong with it?


Diego.

Re: Copy assignments for non scalar types

[Richard Guenther]

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

On Wed, 14 Apr 2010, Diego Novillo wrote:

> On Wed, Apr 14, 2010 at 04:40, Richard Guenther <rguenther@suse.de> wrote:
> 
> > No.  make_rename_temp should go away.  Please.
> 
> I don't disagree, in principle (less code is always good).  What is
> wrong with it?

It asks the SSA renamer to put your new variables into SSA form.
It's very simple to do that manually (at least if no PHIs are
involved), so better do that.

Richard.

Re: Copy assignments for non scalar types

[Martin Jambor]

Hi,

On Wed, Apr 14, 2010 at 01:31:05PM +0200, Richard Guenther wrote:
> On Wed, 14 Apr 2010, Diego Novillo wrote:
> 
> > On Wed, Apr 14, 2010 at 04:40, Richard Guenther <rguenther@suse.de> wrote:
> > 
> > > No.  make_rename_temp should go away.  Please.
> > 
> > I don't disagree, in principle (less code is always good).  What is
> > wrong with it?
> 
> It asks the SSA renamer to put your new variables into SSA form.
> It's very simple to do that manually (at least if no PHIs are
> involved), so better do that.
> 

The problem of using create_tmp_var directly is that the following
pattern is now bound to creep up at quite many places:

      tmp = create_tmp_var (TREE_TYPE (adj->base), "blah");
      if (TREE_CODE (TREE_TYPE (tmp)) == COMPLEX_TYPE
	  || TREE_CODE (TREE_TYPE (tmp)) == VECTOR_TYPE)
	DECL_GIMPLE_REG_P (tmp) = 1;

Perhaps we should have something like create_gimple_reg_tmp_var that
would do this?  If so, I'll be happy to add it.

Martin

Re: Copy assignments for non scalar types

[Richard Guenther]

On Wed, Apr 14, 2010 at 1:44 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Wed, Apr 14, 2010 at 01:31:05PM +0200, Richard Guenther wrote:
>> On Wed, 14 Apr 2010, Diego Novillo wrote:
>>
>> > On Wed, Apr 14, 2010 at 04:40, Richard Guenther <rguenther@suse.de> wrote:
>> >
>> > > No.  make_rename_temp should go away.  Please.
>> >
>> > I don't disagree, in principle (less code is always good).  What is
>> > wrong with it?
>>
>> It asks the SSA renamer to put your new variables into SSA form.
>> It's very simple to do that manually (at least if no PHIs are
>> involved), so better do that.
>>
>
> The problem of using create_tmp_var directly is that the following
> pattern is now bound to creep up at quite many places:
>
>      tmp = create_tmp_var (TREE_TYPE (adj->base), "blah");
>      if (TREE_CODE (TREE_TYPE (tmp)) == COMPLEX_TYPE
>          || TREE_CODE (TREE_TYPE (tmp)) == VECTOR_TYPE)
>        DECL_GIMPLE_REG_P (tmp) = 1;
>
> Perhaps we should have something like create_gimple_reg_tmp_var that
> would do this?  If so, I'll be happy to add it.

Yes.  I suggest create_tmp_reg as a name for that (simply add
a wrapper around create_tmp_var).

Richard.

> Martin
>

Re: Copy assignments for non scalar types

[Diego Novillo]

On Wed, Apr 14, 2010 at 07:31, Richard Guenther <rguenther@suse.de> wrote:

> It asks the SSA renamer to put your new variables into SSA form.
> It's very simple to do that manually (at least if no PHIs are
> involved), so better do that.

But then we'd have lots of duplicate code fragments all doing the same
thing.  Isn't it better to have a single self-contained helper to do
that for us?


Diego.