Initial shrink-wrapping patch

Richard Sandiford richard.sandiford@linaro.org
Thu Sep 1 13:57:00 GMT 2011


Bernd Schmidt <bernds@codesourcery.com> writes:
> +/* A for_each_rtx subroutine of record_hard_reg_sets.  */
> +static int
> +record_hard_reg_uses_1 (rtx *px, void *data)
> +{
> +  rtx x = *px;
> +  HARD_REG_SET *pused = (HARD_REG_SET *)data;
> +
> +  if (REG_P (x) && REGNO (x) < FIRST_PSEUDO_REGISTER)
> +    {
> +      int nregs = hard_regno_nregs[REGNO (x)][GET_MODE (x)];
> +      while (nregs-- > 0)
> +	SET_HARD_REG_BIT (*pused, REGNO (x) + nregs);
> +    }

add_to_hard_reg_set (pused, GET_MODE (x), REGNO (x));

> +/* A subroutine of requires_stack_frame_p, called via for_each_rtx.
> +   If any change is made, set CHANGED
> +   to true.  */

Strange line break, and comment doesn't match code (no "changed" variable).
Probably moot though because:

> +/* Return true if INSN requires the stack frame to be set up.
> +   PROLOGUE_USED contains the hard registers used in the function
> +   prologue.  */
> +static bool
> +requires_stack_frame_p (rtx insn, HARD_REG_SET prologue_used)
> +{
>[...]
> +  if (for_each_rtx (&PATTERN (insn), frame_required_for_rtx, NULL))
> +    return true;
> +  CLEAR_HARD_REG_SET (hardregs);
> +  note_stores (PATTERN (insn), record_hard_reg_sets, &hardregs);
> +  if (hard_reg_set_intersect_p (hardregs, prologue_used))
> +    return true;
> +  AND_COMPL_HARD_REG_SET (hardregs, call_used_reg_set);
> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> +    if (TEST_HARD_REG_BIT (hardregs, regno)
> +	&& df_regs_ever_live_p (regno))
> +      return true;

...I suppose this ought to use DF instead.

Does something guarantee that non-local gotos are marked as
needing a frame?

Why do we need to check specifically for pic_offset_table_rtx?  Isn't it
handled by the generic "live registers set by the prologue" test?
Reason for asking is that pic_offset_table_rtx can be a global value,
such as for mips*-elf.

> -/* Insert gen_return at the end of block BB.  This also means updating
> -   block_for_insn appropriately.  */
> +
> +static rtx
> +gen_return_pattern (bool simple_p)
> +{
> +#ifdef HAVE_simple_return
> +  return simple_p ? gen_simple_return () : gen_return ();
> +#else
> +  gcc_assert (!simple_p);
> +  return gen_return ();
> +#endif
> +}

Missing function comment.

> +
> +/* Insert an appropriate return pattern at the end of block BB.  This
> +   also means updating block_for_insn appropriately.  */
>  
>  static void
> -emit_return_into_block (basic_block bb)
> +emit_return_into_block (bool simple_p, basic_block bb)

Pedantic, but the comment should reference SIMPLE_P.

> +#ifdef HAVE_simple_return
> +  /* Try to perform a kind of shrink-wrapping, making sure the
> +     prologue/epilogue is emitted only around those parts of the
> +     function that require it.  */
> +
> +  if (flag_shrink_wrap && HAVE_simple_return && !flag_non_call_exceptions
> +      && HAVE_prologue && !crtl->calls_eh_return)
> +    {

Maybe it should be obvious, but could you add a comment explaining why
flag_non_call_exceptions and crtl->calls_eh_return need to be checked
explicitly?  I can see why we don't want to optimise functions that
call eh_return, but I was curious why it wasn't handled by the general
insn-level analysis.

Would checking prologue_seq != NULL be better than HAVE_prologue?

> +      HARD_REG_SET prologue_clobbered, prologue_used, live_on_edge;
> +      rtx p_insn;
> +
> +      VEC(basic_block, heap) *vec;
> +      basic_block bb;
> +      bitmap_head bb_antic_flags;
> +      bitmap_head bb_on_list;
> +
> +      /* Compute the registers set and used in the prologue.  */
> +      CLEAR_HARD_REG_SET (prologue_clobbered);
> +      CLEAR_HARD_REG_SET (prologue_used);
> +      for (p_insn = prologue_seq; p_insn; p_insn = NEXT_INSN (p_insn))
> +	{
> +	  HARD_REG_SET this_used;
> +	  if (!NONDEBUG_INSN_P (p_insn))
> +	    continue;
> +
> +	  CLEAR_HARD_REG_SET (this_used);
> +	  note_uses (&PATTERN (p_insn), record_hard_reg_uses,
> +		     &this_used);
> +	  AND_COMPL_HARD_REG_SET (this_used, prologue_clobbered);
> +	  IOR_HARD_REG_SET (prologue_used, this_used);
> +	  note_stores (PATTERN (p_insn), record_hard_reg_sets,
> +		       &prologue_clobbered);
> +	}

Should this iterate over split_prologue_seq as well?

Could we combine prologue_seq and split_prologue_seq into a single sequence?

> +      bitmap_initialize (&bb_antic_flags, &bitmap_default_obstack);
> +      bitmap_initialize (&bb_on_list, &bitmap_default_obstack);
> +
> +      vec = VEC_alloc (basic_block, heap, n_basic_blocks);
> +
> +      FOR_EACH_BB (bb)
> +	{
> +	  rtx insn;
> +	  FOR_BB_INSNS (bb, insn)
> +	    {
> +	      if (requires_stack_frame_p (insn, prologue_used))
> +		{
> +		  bitmap_set_bit (&bb_flags, bb->index);
> +		  VEC_quick_push (basic_block, vec, bb);
> +		  break;
> +		}
> +	    }

Excess { and } in for loop.

> +	}
> +
> +      /* For every basic block that needs a prologue, mark all blocks
> +	 reachable from it, so as to ensure they are also seen as
> +	 requiring a prologue.  */
> +      while (!VEC_empty (basic_block, vec))
> +	{
> +	  basic_block tmp_bb = VEC_pop (basic_block, vec);
> +	  edge e;
> +	  edge_iterator ei;
> +	  FOR_EACH_EDGE (e, ei, tmp_bb->succs)
> +	    {
> +	      if (e->dest == EXIT_BLOCK_PTR
> +		  || bitmap_bit_p (&bb_flags, e->dest->index))
> +		continue;
> +	      bitmap_set_bit (&bb_flags, e->dest->index);
> +	      VEC_quick_push (basic_block, vec, e->dest);
> +	    }

Don't know which is the preferred style, but:

    if (e->dest != EXIT_BLOCK_PTR
        && bitmap_set_bit (&bb_flags, e->dest->index))
      VEC_quick_push (basic_block, vec, e->dest);

should be a little more efficient.

> +      /* Now walk backwards from every block that is marked as needing
> +	 a prologue to compute the bb_antic_flags bitmap.  */
> +      bitmap_copy (&bb_antic_flags, &bb_flags);
> +      FOR_EACH_BB (bb)
> +	{
> +	  edge e;
> +	  edge_iterator ei;
> +	  if (!bitmap_bit_p (&bb_flags, bb->index))
> +	    continue;
> +	  FOR_EACH_EDGE (e, ei, bb->preds)
> +	    if (!bitmap_bit_p (&bb_antic_flags, e->src->index))
> +	      {
> +		VEC_quick_push (basic_block, vec, e->src);
> +		bitmap_set_bit (&bb_on_list, e->src->index);
> +	      }

Here too I think we want:

	  FOR_EACH_EDGE (e, ei, bb->preds)
	    if (!bitmap_bit_p (&bb_antic_flags, e->src->index)
		&& bitmap_set_bit (&bb_on_list, e->src->index))
	      VEC_quick_push (basic_block, vec, e->src);

in this case to avoid pushing the same thing twice.

> +	  bitmap_clear_bit (&bb_on_list, tmp_bb->index);
> +	  FOR_EACH_EDGE (e, ei, tmp_bb->succs)
> +	    {
> +	      if (!bitmap_bit_p (&bb_antic_flags, e->dest->index))
> +		{
> +		  all_set = false;
> +		  break;
> +		}
> +	    }

Excess { and } in for loop.

> +	  if (all_set)
> +	    {
> +	      bitmap_set_bit (&bb_antic_flags, tmp_bb->index);
> +	      FOR_EACH_EDGE (e, ei, tmp_bb->preds)
> +		if (!bitmap_bit_p (&bb_antic_flags, e->src->index))
> +		  {
> +		    VEC_quick_push (basic_block, vec, e->src);
> +		    bitmap_set_bit (&bb_on_list, e->src->index);
> +		  }
> +	    }

Same:

	  FOR_EACH_EDGE (e, ei, tmp_bb->preds)
	    if (!bitmap_bit_p (&bb_antic_flags, e->src->index)
		&& bitmap_set_bit (&bb_on_list, e->src->index))
	      VEC_quick_push (basic_block, vec, e->src);

comment as above.

> +      /* Test whether the prologue is known to clobber any register
> +	 (other than FP or SP) which are live on the edge.  */
> +      CLEAR_HARD_REG_SET (prologue_clobbered);
> +      for (p_insn = prologue_seq; p_insn; p_insn = NEXT_INSN (p_insn))
> +	if (NONDEBUG_INSN_P (p_insn))
> +	  note_stores (PATTERN (p_insn), record_hard_reg_sets,
> +		       &prologue_clobbered);
> +      for (p_insn = split_prologue_seq; p_insn; p_insn = NEXT_INSN (p_insn))
> +	if (NONDEBUG_INSN_P (p_insn))
> +	  note_stores (PATTERN (p_insn), record_hard_reg_sets,
> +		       &prologue_clobbered);
> +      CLEAR_HARD_REG_BIT (prologue_clobbered, STACK_POINTER_REGNUM);
> +      if (frame_pointer_needed)
> +	CLEAR_HARD_REG_BIT (prologue_clobbered, HARD_FRAME_POINTER_REGNUM);

Seems like we should be able to reuse the insn walk from the beginning
of the enclosing if statement.

> +#ifdef HAVE_simple_return
> +	  simple_p = (entry_edge != orig_entry_edge
> +		      ? !bitmap_bit_p (&bb_flags, bb->index) : false);
> +#else
> +	  simple_p = false;
> +#endif

Why is this bb_flags rather than bb_antic_flags?

	  simple_p = (entry_edge != orig_entry_edge
		      && !bitmap_bit_p (&bb_flags, bb->index));

seems more readable, but I suppose that's personal taste.

> +	      gcc_assert (simple_p);
> +	      new_bb = split_edge (e);
> +	      emit_barrier_after (BB_END (new_bb));
> +	      emit_return_into_block (simple_p, new_bb);
> +#ifdef HAVE_simple_return
> +	      if (simple_p)

Check is redundant given the gcc_assert.


> +	      /* If this block has only one successor, it both jumps
> +		 and falls through to the fallthru block, so we can't
> +		 delete the edge.  */
> +	      if (single_succ_p (bb))
> +		continue;

Seems odd that this could happen in optimised code, but if it did,
wouldn't it invalidate the simple_p transformation?  Seems like
the non-fallthrough edge would use a simple return but the
fallthrough one wouldn't.

> -      start_sequence ();
> -      emit_note (NOTE_INSN_EPILOGUE_BEG);
> -      emit_insn (gen_sibcall_epilogue ());
> -      seq = get_insns ();
> -      end_sequence ();
> +      ep_seq = gen_sibcall_epilogue ();
> +      if (ep_seq)
> +	{

Why the new null check?

Richard



More information about the Gcc-patches mailing list