what can be in a group set?

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

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

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

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

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

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