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
