what can be in a group set?

what can be in a group set?

[Paolo Bonzini]

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

df-scan.c has this code to deal with group sets:

   /* It is legal to have a set destination be a parallel. */
   if (GET_CODE (dst) == PARALLEL)
     {
       int i;

       for (i = XVECLEN (dst, 0) - 1; i >= 0; i--)
         {
           rtx temp = XVECEXP (dst, 0, i);
           if (GET_CODE (temp) == EXPR_LIST || GET_CODE (temp) == CLOBBER
               || GET_CODE (temp) == SET)
             df_def_record_1 (collection_rec,
                              temp, bb, insn_info,
                              GET_CODE (temp) == CLOBBER
                              ? flags | DF_REF_MUST_CLOBBER : flags);
         }
       return;
     }

It seems to me that the case of (set (parallel [(set ...)])) and (set 
(parallel [(clobber ...)])) is bogus.  I would like to simplify it to 
the following:

   /* It is legal to have a set destination be a parallel. */
   if (GET_CODE (dst) == PARALLEL)
     {
       int i;

       for (i = XVECLEN (dst, 0) - 1; i >= 0; i--)
         {
           rtx temp = XVECEXP (dst, 0, i);
           assert (GET_CODE (temp) == EXPR_LIST);
           df_def_record_1 (collection_rec, temp, bb, insn_info, flags);
         }
       return;
     }

Does this make sense?  See the attached patch for the overall thing I 
was thinking of.

Paolo

[-- Attachment #2: cleanup-df-ref-record.patch --]
[-- Type: text/x-patch, Size: 3000 bytes --]

        * df-scan.c (df_def_record_1): Assert a parallel must contain
        an EXPR_LIST at this point.  Receive the LOC and move its
        extraction...
        (df_defs_record): ... here.  Remove superfluous braces.

Index: df-scan.c
===================================================================
--- df-scan.c	(revision 169877)
+++ df-scan.c	(working copy)
@@ -111,7 +111,7 @@ static void df_ref_record (enum df_ref_c
 			   rtx, rtx *,
 			   basic_block, struct df_insn_info *,
 			   enum df_ref_type, int ref_flags);
-static void df_def_record_1 (struct df_collection_rec *, rtx,
+static void df_def_record_1 (struct df_collection_rec *, rtx *,
 			     basic_block, struct df_insn_info *,
 			     int ref_flags);
 static void df_defs_record (struct df_collection_rec *, rtx,
@@ -2922,19 +2922,10 @@ df_read_modify_subreg_p (rtx x)
 
 static void
 df_def_record_1 (struct df_collection_rec *collection_rec,
-                 rtx x, basic_block bb, struct df_insn_info *insn_info,
+                 rtx *loc, basic_block bb, struct df_insn_info *insn_info,
 		 int flags)
 {
-  rtx *loc;
-  rtx dst;
-
- /* We may recursively call ourselves on EXPR_LIST when dealing with PARALLEL
-     construct.  */
-  if (GET_CODE (x) == EXPR_LIST || GET_CODE (x) == CLOBBER)
-    loc = &XEXP (x, 0);
-  else
-    loc = &SET_DEST (x);
-  dst = *loc;
+  rtx dst = *loc;
 
   /* It is legal to have a set destination be a parallel. */
   if (GET_CODE (dst) == PARALLEL)
@@ -2944,12 +2935,9 @@ df_def_record_1 (struct df_collection_re
       for (i = XVECLEN (dst, 0) - 1; i >= 0; i--)
 	{
 	  rtx temp = XVECEXP (dst, 0, i);
-	  if (GET_CODE (temp) == EXPR_LIST || GET_CODE (temp) == CLOBBER
-	      || GET_CODE (temp) == SET)
-	    df_def_record_1 (collection_rec,
-                             temp, bb, insn_info,
-			     GET_CODE (temp) == CLOBBER
-			     ? flags | DF_REF_MUST_CLOBBER : flags);
+	  gcc_assert (GET_CODE (temp) == EXPR_LIST);
+	  df_def_record_1 (collection_rec, &XEXP (temp, 0),
+			   bb, insn_info, flags);
 	}
       return;
     }
@@ -3003,18 +2991,16 @@ df_defs_record (struct df_collection_rec
 {
   RTX_CODE code = GET_CODE (x);
 
-  if (code == SET || code == CLOBBER)
-    {
-      /* Mark the single def within the pattern.  */
-      int clobber_flags = flags;
-      clobber_flags |= (code == CLOBBER) ? DF_REF_MUST_CLOBBER : 0;
-      df_def_record_1 (collection_rec, x, bb, insn_info, clobber_flags);
-    }
+  if (code == SET)
+    df_def_record_1 (collection_rec, &SET_DEST (x), bb, insn_info, flags);
+  else if (code == CLOBBER)
+    {
+      flags |= DF_REF_MUST_CLOBBER;
+      df_def_record_1 (collection_rec, &XEXP (x, 0), bb, insn_info, flags);
+    }
   else if (code == COND_EXEC)
-    {
-      df_defs_record (collection_rec, COND_EXEC_CODE (x),
-		      bb, insn_info, DF_REF_CONDITIONAL);
-    }
+    df_defs_record (collection_rec, COND_EXEC_CODE (x),
+		    bb, insn_info, DF_REF_CONDITIONAL);
   else if (code == PARALLEL)
     {
       int i;

Re: what can be in a group set?

[Richard Sandiford]

Paolo Bonzini <bonzini@gnu.org> writes:
> df-scan.c has this code to deal with group sets:
>
>    /* It is legal to have a set destination be a parallel. */
>    if (GET_CODE (dst) == PARALLEL)
>      {
>        int i;
>
>        for (i = XVECLEN (dst, 0) - 1; i >= 0; i--)
>          {
>            rtx temp = XVECEXP (dst, 0, i);
>            if (GET_CODE (temp) == EXPR_LIST || GET_CODE (temp) == CLOBBER
>                || GET_CODE (temp) == SET)
>              df_def_record_1 (collection_rec,
>                               temp, bb, insn_info,
>                               GET_CODE (temp) == CLOBBER
>                               ? flags | DF_REF_MUST_CLOBBER : flags);
>          }
>        return;
>      }
>
> It seems to me that the case of (set (parallel [(set ...)])) and (set 
> (parallel [(clobber ...)])) is bogus.  I would like to simplify it to 
> the following:
>
>    /* It is legal to have a set destination be a parallel. */
>    if (GET_CODE (dst) == PARALLEL)
>      {
>        int i;
>
>        for (i = XVECLEN (dst, 0) - 1; i >= 0; i--)
>          {
>            rtx temp = XVECEXP (dst, 0, i);
>            assert (GET_CODE (temp) == EXPR_LIST);
>            df_def_record_1 (collection_rec, temp, bb, insn_info, flags);
>          }
>        return;
>      }
>
> Does this make sense?

Yeah, the docs seem pretty certain that expr_list is the only valid choice.

The docs also say that the first expr_list can be null:

  If @var{lval} is a @code{parallel}, it is used to represent the case of
  a function returning a structure in multiple registers.  Each element
  of the @code{parallel} is an @code{expr_list} whose first operand is a
  @code{reg} and whose second operand is a @code{const_int} representing the
  offset (in bytes) into the structure at which the data in that register
  corresponds.  The first element may be null to indicate that the structure
  is also passed partly in memory.

but I can't see any code to handle that.  Am I missing something,
or does the lack of a crash here mean that we can remove the last
sentence?

(It might have been added for symmetry with argument passing, where this
sort of thing is needed.  But if it isn't actually used or implemented for
returns, it might be less confusing to remove it.)

> See the attached patch for the overall thing I 
> was thinking of.
>
> Paolo
>
>         * df-scan.c (df_def_record_1): Assert a parallel must contain
>         an EXPR_LIST at this point.  Receive the LOC and move its
>         extraction...
>         (df_defs_record): ... here.  Remove superfluous braces.

Looks good.

Richard

Re: what can be in a group set?

[Paolo Bonzini]

On 07/08/2011 12:43 PM, Richard Sandiford wrote:
> The docs also say that the first expr_list can be null:
>
>    If @var{lval} is a @code{parallel}, it is used to represent the case of
>    a function returning a structure in multiple registers.  Each element
>    of the @code{parallel} is an @code{expr_list} whose first operand is a
>    @code{reg} and whose second operand is a @code{const_int} representing the
>    offset (in bytes) into the structure at which the data in that register
>    corresponds.  The first element may be null to indicate that the structure
>    is also passed partly in memory.
>
> but I can't see any code to handle that.  Am I missing something,
> or does the lack of a crash here mean that we can remove the last
> sentence?
>
> (It might have been added for symmetry with argument passing, where this
> sort of thing is needed.  But if it isn't actually used or implemented for
> returns, it might be less confusing to remove it.)

Indeed.  Dimitrios, can you pick up the patch since it will somewhat 
simplify your work to eliminate defs_generated?

Paolo

Re: what can be in a group set?

[Dimitrios Apostolou]

On Fri, 8 Jul 2011, Paolo Bonzini wrote:
> On 07/08/2011 12:43 PM, Richard Sandiford wrote:
>> The docs also say that the first expr_list can be null:
>>
>>    If @var{lval} is a @code{parallel}, it is used to represent the case of
>>    a function returning a structure in multiple registers.  Each element
>>    of the @code{parallel} is an @code{expr_list} whose first operand is a
>>    @code{reg} and whose second operand is a @code{const_int} representing 
>> the
>>    offset (in bytes) into the structure at which the data in that register
>>    corresponds.  The first element may be null to indicate that the 
>> structure
>>    is also passed partly in memory.
>> 
>> but I can't see any code to handle that.  Am I missing something,
>> or does the lack of a crash here mean that we can remove the last
>> sentence?
>> 
>> (It might have been added for symmetry with argument passing, where this
>> sort of thing is needed.  But if it isn't actually used or implemented for
>> returns, it might be less confusing to remove it.)
>
> Indeed.  Dimitrios, can you pick up the patch since it will somewhat simplify 
> your work to eliminate defs_generated?

I'll certainly try :-)

Paolo, something else, in df_mark_reg() is it ever possible for regno to 
be >= FIRST_PSEUDO_REGISTER? An assert I've put doesn't trigger for my 
simple test :-)


Thanks,
Dimitris

Re: what can be in a group set?

[Paolo Bonzini]

On 07/08/2011 03:05 PM, Dimitrios Apostolou wrote:
> 
> Paolo, something else, in df_mark_reg() is it ever possible for regno to 
> be >= FIRST_PSEUDO_REGISTER? An assert I've put doesn't trigger for my 
> simple test :-)

From reading the docs of EH_RETURN_STACKADJ_RTX and EH_RETURN_HANDLER_RTX,
it seems you're safe.

This in df-problems.c also suggests the same:

      if (bb_index == EXIT_BLOCK)
        {
          unsigned regno;
          bitmap_iterator bi;
          EXECUTE_IF_SET_IN_BITMAP (df->exit_block_uses, FIRST_PSEUDO_REGISTER,
                                    regno, bi)
            gcc_unreachable ();
        }

A more solid reasoning is that a pseudo cannot be considered live at exit or
at entry to a function, because the caller would not know where it lives.

That said, changing exit_block_uses and entry_block_defs to HARD_REG_SET would
be a nice cleanup, but it would also touch target code due to

  targetm.extra_live_on_entry (entry_block_defs);

I wouldn't bother for now until you're a bit more experienced.  Unlike
invalidated_by_call it shouldn't show up in profiles, or does it?

Paolo

Re: what can be in a group set?

[Dimitrios Apostolou]

Thanks Paolo for the detailed explanation!

On Fri, 8 Jul 2011, Paolo Bonzini wrote:
>
> That said, changing exit_block_uses and entry_block_defs to HARD_REG_SET would
> be a nice cleanup, but it would also touch target code due to
>
>  targetm.extra_live_on_entry (entry_block_defs);
>

I've already done that :-p

> I wouldn't bother for now until you're a bit more experienced.  Unlike
> invalidated_by_call it shouldn't show up in profiles, or does it?

Indeed it doesn't show, I just wanted to do it as a clean-up for 
transitioning to HARD_REG_SET all relevant sets in struct df_d.

The only problem remaining is I need a bitmap_copy_from_hard_reg_set() 
function for df_lr_local_compute(), where the bb_info->use bitmap is 
initialised from the exit_block_uses HARD_REG_SET.


Dimitris