Patchwork SSA identifiers

login
register
mail settings
Submitter Andrew MacLeod
Date June 27, 2013, 6:25 p.m.
Message ID <51CC8391.5000900@redhat.com>
Download mbox | patch
Permalink /patch/255119/
State New
Headers show

Comments

Andrew MacLeod - June 27, 2013, 6:25 p.m.
I'm doing some trial file conversions to my proposed wrapper classes,  
I'm seeing a couple of places which aren't mapping properly (they 
triggered compile errors),  and I think its  because there is a couple 
of bugs regarding  the use of IDENTIFIERS in ssanames..   (I wasn't even 
aware you could have non-var_decls...) so this is just a sanity check :-)


the macros are defined as:

/* Returns the IDENTIFIER_NODE giving the SSA name a name or NULL_TREE
    if there is no name associated with it.  */
#define SSA_NAME_IDENTIFIER(NODE)                               \
   (SSA_NAME_CHECK (NODE)->ssa_name.var != NULL_TREE             \
    ? (TREE_CODE ((NODE)->ssa_name.var) == IDENTIFIER_NODE       \
       ? (NODE)->ssa_name.var                                    \
       : DECL_NAME ((NODE)->ssa_name.var))                       \
    : NULL_TREE)

/* Returns the variable being referenced.  This can be NULL_TREE for
    temporaries not associated with any user variable.
    Once released, this is the only field that can be relied upon. */
#define SSA_NAME_VAR(NODE)                                      \
   (SSA_NAME_CHECK (NODE)->ssa_name.var == NULL_TREE             \
    || TREE_CODE ((NODE)->ssa_name.var) == IDENTIFIER_NODE       \
    ? NULL_TREE : (NODE)->ssa_name.var)

#define SET_SSA_NAME_VAR_OR_IDENTIFIER(NODE,VAR) \
   do { SSA_NAME_CHECK (NODE)->ssa_name.var = (VAR); } while (0)


in tree-ssanames.c:release_ssa_names() :

if (! SSA_NAME_IN_FREE_LIST (var))
     {
       tree saved_ssa_name_var = SSA_NAME_VAR (var);
       int saved_ssa_name_version = SSA_NAME_VERSION (var);
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
<..>
/* Hopefully this can go away once we have the new incremental
          SSA updating code installed.  */
       SET_SSA_NAME_VAR_OR_IDENTIFIER (var, saved_ssa_name_var);

It would seem to me that if the identifier is set rather than the 
vardecl, it is not going  to be reset properly here since the saved 
value is going to be NULL rather than the identifier node...    Or does 
it really matter here? Its not really clear to me.


in tree-outof-ssa.c:insert_backedge_copies() :

for (i = 0; i < gimple_phi_num_args (phi); i++)
             {
               tree arg = gimple_phi_arg_def (phi, i);
               edge e = gimple_phi_arg_edge (phi, i);

               /* If the argument is not an SSA_NAME, then we will need a
                  constant initialization.  If the argument is an 
SSA_NAME with
                  a different underlying variable then a copy statement 
will be
                  needed.  */
               if ((e->flags & EDGE_DFS_BACK)
                   && (TREE_CODE (arg) != SSA_NAME
                       || SSA_NAME_VAR (arg) != SSA_NAME_VAR (result)
                       || trivially_conflicts_p (bb, result, arg)))

If both SSA_NAMEs have an identifier, then   "SSA_NAME_VAR(arg) != 
SSA_NAME_VAR(result)"   is going to compare NULL_TREE != NULL_TREE every 
time...

  I would say its really looking to compare that underlying field
     TREE_CODE ((arg)->ssa_name.var != TREE_CODE ((result)->ssa_name.var

They could both be resolved by adding an equivalent
#define SSA_NAME_VAR_OR_IDENTIFIER(NODE  (SSA_NAME_CHECK 
(NODE)->ssa_name.var)
and using that macro instead of SSA_NAME_VAR.. and  probably more 
efficient.  something like that attached, which bootstraps but I haven't 
done a full testrun yet...

Or perhaps even just use SSA_NAME_IDENTIFIER in both cases would 
work...  seems dangerous if 2 different decls have the same identifier tho.

It is interesting how these 2 cases just popped up as errors building 
during the conversion of those files...  Early tangible benefit? :-)

Andrew
* tree.h (SSA_NAME_VAR_OR_IDENTIFIER): New.
	* tree-ssanames.c: Use instead of SSA_NAME_VAR.
	* tree-outof-ssa.c: Use instead of SSA_NAME_VAR.
Jakub Jelinek - June 27, 2013, 6:39 p.m.
On Thu, Jun 27, 2013 at 02:25:21PM -0400, Andrew MacLeod wrote:
> I'm doing some trial file conversions to my proposed wrapper
> classes,  I'm seeing a couple of places which aren't mapping
> properly (they triggered compile errors),  and I think its  because
> there is a couple of bugs regarding  the use of IDENTIFIERS in
> ssanames..   (I wasn't even aware you could have non-var_decls...)
> so this is just a sanity check :-)

If ssa_name.var is IDENTIFIER_NODE, it is just like ssa_name.var == NULL,
i.e. anonymous SSA_NAME, just that in the dumps it is given some name
and not just _NNN.

> in tree-ssanames.c:release_ssa_names() :
> 
> if (! SSA_NAME_IN_FREE_LIST (var))
>     {
>       tree saved_ssa_name_var = SSA_NAME_VAR (var);
>       int saved_ssa_name_version = SSA_NAME_VERSION (var);
>       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
> <..>
> /* Hopefully this can go away once we have the new incremental
>          SSA updating code installed.  */
>       SET_SSA_NAME_VAR_OR_IDENTIFIER (var, saved_ssa_name_var);

I don't see a big issue with this, sure, you could
tree saved_ssa_name_identifier = saved_ssa_name_var ? saved_ssa_name_var : SSA_NAME_IDENTIFIER (var);
and use that instead in SET_SSA_NAME_VAR_OR_IDENTIFIER.

	Jakub
Andrew MacLeod - June 27, 2013, 6:52 p.m.
On 06/27/2013 02:39 PM, Jakub Jelinek wrote:
>
> in tree-ssanames.c:release_ssa_names() :
>
> if (! SSA_NAME_IN_FREE_LIST (var))
>      {
>        tree saved_ssa_name_var = SSA_NAME_VAR (var);
>        int saved_ssa_name_version = SSA_NAME_VERSION (var);
>        use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
> <..>
> /* Hopefully this can go away once we have the new incremental
>           SSA updating code installed.  */
>        SET_SSA_NAME_VAR_OR_IDENTIFIER (var, saved_ssa_name_var);
> I don't see a big issue with this, sure, you could
> tree saved_ssa_name_identifier = saved_ssa_name_var ? saved_ssa_name_var : SSA_NAME_IDENTIFIER (var);
> and use that instead in SET_SSA_NAME_VAR_OR_IDENTIFIER.

Yeah I wasn't too concerned about this one, the outof-ssa case looked 
like more of a possible issue.  Maybe neither is, they just popped out 
as inconsistent uses.

Andrew
Richard Guenther - Aug. 27, 2013, 7:54 a.m.
On Thu, Jun 27, 2013 at 8:52 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> On 06/27/2013 02:39 PM, Jakub Jelinek wrote:
>>
>>
>> in tree-ssanames.c:release_ssa_names() :
>>
>> if (! SSA_NAME_IN_FREE_LIST (var))
>>      {
>>        tree saved_ssa_name_var = SSA_NAME_VAR (var);
>>        int saved_ssa_name_version = SSA_NAME_VERSION (var);
>>        use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
>> <..>
>> /* Hopefully this can go away once we have the new incremental
>>           SSA updating code installed.  */
>>        SET_SSA_NAME_VAR_OR_IDENTIFIER (var, saved_ssa_name_var);
>> I don't see a big issue with this, sure, you could
>> tree saved_ssa_name_identifier = saved_ssa_name_var ? saved_ssa_name_var :
>> SSA_NAME_IDENTIFIER (var);
>> and use that instead in SET_SSA_NAME_VAR_OR_IDENTIFIER.
>
>
> Yeah I wasn't too concerned about this one, the outof-ssa case looked like
> more of a possible issue.  Maybe neither is, they just popped out as
> inconsistent uses.

Restoring SSA_NAME_VAR_OR_IDENTIFIER is only for debugging.  Yes,
it probably should save SSA_NAME_VAR_OR_IDENTIFIER instead of SSA_NAME_VAR.

Richard.

> Andrew
>

Patch

Index: tree.h
===================================================================
--- tree.h	(revision 196663)
+++ tree.h	(working copy)
@@ -1917,6 +1918,8 @@ 
    || TREE_CODE ((NODE)->ssa_name.var) == IDENTIFIER_NODE	\
    ? NULL_TREE : (NODE)->ssa_name.var)
 
+#define SSA_NAME_VAR_OR_IDENTIFIER(NODE) (SSA_NAME_CHECK (NODE)->ssa_name.var)
+
 #define SET_SSA_NAME_VAR_OR_IDENTIFIER(NODE,VAR) \
   do { SSA_NAME_CHECK (NODE)->ssa_name.var = (VAR); } while (0)
 
Index: tree-ssanames.c
===================================================================
--- tree-ssanames.c	(revision 195512)
+++ tree-ssanames.c	(working copy)
@@ -200,7 +200,7 @@ 
      defining statement.  */
   if (! SSA_NAME_IN_FREE_LIST (var))
     {
-      tree saved_ssa_name_var = SSA_NAME_VAR (var);
+      tree saved_ssa_name_var = SSA_NAME_VAR_OR_IDENTIFIER (var);
       int saved_ssa_name_version = SSA_NAME_VERSION (var);
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
 
Index: tree-outof-ssa.c
===================================================================
--- tree-outof-ssa.c	(revision 195512)
+++ tree-outof-ssa.c	(working copy)
@@ -1039,7 +1039,8 @@ 
 		 needed.  */
 	      if ((e->flags & EDGE_DFS_BACK)
 		  && (TREE_CODE (arg) != SSA_NAME
-		      || SSA_NAME_VAR (arg) != SSA_NAME_VAR (result)
+		      || (SSA_NAME_VAR_OR_IDENTIFIER (arg) 
+			  != SSA_NAME_VAR_OR_IDENTIFIER  (result))
 		      || trivially_conflicts_p (bb, result, arg)))
 		{
 		  tree name;