# what can be in a group set?

## Commit Message

Paolo Bonzini July 8, 2011, 7:11 a.m.
```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
```

Richard Sandiford July 8, 2011, 10:43 a.m. | #1
```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
```
Paolo Bonzini July 8, 2011, 12:56 p.m. | #2
```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
```
Dimitrios Apostolou July 8, 2011, 1:05 p.m. | #3
```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
```
Paolo Bonzini July 8, 2011, 1:22 p.m. | #4
```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 :-)

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
```
Dimitrios Apostolou July 8, 2011, 1:37 p.m. | #5
```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 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
```

## Patch

```        * 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;

```