[PATCH] Set stores_off_frame_dead_at_return to false if sibling call

[Jeff Law]

On 12/01/14 11:44, John David Anglin wrote:
>>
>> To do unconditionally set frame_read?  Or if we don't want to get
>> that drastic,
>>
>> if (reload_completed || SIBLING_CALL_P (insn))
>> insn_info->frame_read = true;
> Will test but I, if I read the code correctly, setting
> insn_info->frame_read = true results in frame related stores being
> killed in a constant call. This doesn't seem like the right
> solution.
But isn't killing in this context referring to GEN/KILL types of 
operations for global dataflow?   ISTM that GEN is a store operation 
while KILL is a load operation (over-simplification, but stick with me).

Thus a GEN-GEN will get optimized into a single GEN (dead store 
eliminated) while a GEN-KILL-GEN can not be optimized by DSE because of 
the KILL.

It should always be safe to have an extraneous KILL since that merely 
inhibits optimization.  While an extraneous GEN can be a disaster.

So with setting frame_read, we're going to have more KILLs in the 
dataflow sets, which results in fewer stores being eliminated.  They 
come from:


               /* If the frame is read, the frame related stores are 
killed.  */
               else if (insn_info->frame_read)
                 {
                   store_info_t store_info = i_ptr->store_rec;

                   /* Skip the clobbers.  */
                   while (!store_info->is_set)
                     store_info = store_info->next;

                   if (store_info->group_id >= 0
                       && 
rtx_group_vec[store_info->group_id]->frame_related)
                     remove_store = true;
                 }

               if (remove_store)
                 {
                   if (dump_file && (dump_flags & TDF_DETAILS))
                     dump_insn_info ("removing from active", i_ptr);

                   active_local_stores_len--;
                   if (last)
                     last->next_local_store = i_ptr->next_local_store;
                   else
                     active_local_stores = i_ptr->next_local_store;
                 }
               else
                 last = i_ptr;

               i_ptr = i_ptr->next_local_store;

AFAICT in this loop, setting FRAME_READ causes us to set REMOVE_STORE 
more often.  REMOVE_STORE in this context seems to indicate that we're 
going to remove a store from the list we are tracking for potential 
removal.  Which seems exactly right.

Here as well:

   /* If this insn reads the frame, kill all the frame related stores.  */
   if (insn_info->frame_read)
     {
       FOR_EACH_VEC_ELT (rtx_group_vec, i, group)
         if (group->process_globally && group->frame_related)
           {
             if (kill)
               bitmap_ior_into (kill, group->group_kill);
             bitmap_and_compl_into (gen, group->group_kill);
           }
     }

Which also seems like exactly what we want since when we set FRAME_READ 
we end up with a bigger KILL set and a smaller GEN set.


I think the terminology and variable names certainly makes this tougher 
to follow than it should.


>
> Here we have frame related calls being killed before reload because
> the argument stores for the sibcall are off frame:
>
> /* Set the gen set of the exit block, and also any block with no
> successors that does not have a wild read.  */
>
> static void dse_step3_exit_block_scan (bb_info_t bb_info) { /* The
> gen set is all 0's for the exit block except for the
> frame_pointer_group.  */
>
> if (stores_off_frame_dead_at_return) { unsigned int i; group_info_t
> group;
>
> FOR_EACH_VEC_ELT (rtx_group_vec, i, group) { if
> (group->process_globally && group->frame_related) bitmap_ior_into
> (bb_info->gen, group->group_kill); } } }
I see your point.  Expanding the GEN set here seems wrong.    If we go 
with setting STORES_OFF_FRAME_DEAD_AT_RETURN, then we definitely need to 
update its documentation.

I think an argument could be made that we want both changes if I've 
interpreted this code correctly.

Jeff