Re: [PATCH 8/9] shrink-wrap: shrink-wrapping for separate concerns

[Bernd Schmidt <bschmidt@redhat.com>]

I still have misgivings about all the changes needed to the following 
passes, but I guess there's no choice but to live with it. So, I'm 
trying to look at this patch, but I'm finding it fairly impenetrable and 
underdocumented.

> +  /* The concerns for which we want a prologue placed on this BB.  */
> +  sbitmap pro_concern;
> +
> +  /* The concerns for which we placed code at the start of the BB.  */
> +  sbitmap head_concern;

What's the difference?

> +  /* The frequency of executing the prologue for this BB and all BBs
> +     dominated by it.  */
> +  gcov_type cost;

Is this frequency consideration the only thing that attempts to prevent 
placing prologue insns into loops?

> +
> +/* Destroy the pass-specific data.  */
> +static void
> +fini_separate_shrink_wrap (void)
> +{
> +  basic_block bb;
> +  FOR_ALL_BB_FN (bb, cfun)
> +    if (bb->aux)
> +      {
> +	sbitmap_free (SW (bb)->has_concern);
> +	sbitmap_free (SW (bb)->pro_concern);
> +	sbitmap_free (SW (bb)->head_concern);
> +	sbitmap_free (SW (bb)->tail_concern);
> +	free (bb->aux);
> +	bb->aux = 0;
> +      }
> +}

Almost makes me want to ask for an sbitmap variant allocated on obstacks.

> +      /* If this block does have the concern itself, or it is cheaper to
> +         put the prologue here than in all the descendants that need it,
> +	 mark it so.  If it is the same cost, put it here if there is no
> +	 block reachable from this block that does not need the prologue.
> +	 The actual test is a bit more stringent but catches most cases.  */

There's some oddness here with the leading whitespace.

> +/* Mark HAS_CONCERN for every block dominated by at least one block with
> +   PRO_CONCERN set, starting at HEAD.  */

I see a lot of code dealing with the placement of prologue 
parts/concerns/components, but very little dealing with how to place 
epilogues, leading me to wonder whether we could do better wrt the 
latter. Shouldn't there be some mirror symmetry, i.e. 
spread_concerns_backwards?

> +    {
> +      if (first_visit)
> +	{
> +	  bitmap_ior (SW (bb)->has_concern, SW (bb)->pro_concern, concern);
> +
> +	  if (first_dom_son (CDI_DOMINATORS, bb))
> +	    {
> +	      concern = SW (bb)->has_concern;
> +	      bb = first_dom_son (CDI_DOMINATORS, bb);
> +	      continue;
> +	    }

Calling first_dom_son twice with the same args? More importantly, this 
first_visit business seems very confusing. I'd try to find a way to 
merge this if with the places that set first_visit to true. Also - 
instead of having a "continue;" here it seems the code inside the if 
represents an inner loop that should be written explicitly. There are 
two loops with such a structure.

> +/* If we cannot handle placing some concern's prologues or epilogues where
> +   we decided we should place them, unmark that concern in CONCERNS so
> +   that it is not wrapped separately.  */
> +static void
> +disqualify_problematic_concerns (sbitmap concerns)
> +{
> +  sbitmap pro = sbitmap_alloc (SBITMAP_SIZE (concerns));
> +  sbitmap epi = sbitmap_alloc (SBITMAP_SIZE (concerns));
> +
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, cfun)
> +    {
> +      edge e;
> +      edge_iterator ei;
> +      FOR_EACH_EDGE (e, ei, bb->succs)
> +	{
> +	  bitmap_and_compl (epi, SW (e->src)->has_concern,
> +			    SW (e->dest)->has_concern);
> +	  bitmap_and_compl (pro, SW (e->dest)->has_concern,
> +			    SW (e->src)->has_concern);

What is the purpose of this?

> +/* Place code for prologues and epilogues for CONCERNS where we can put
> +   that code at the start of basic blocks.  */
> +static void
> +do_common_heads_for_concerns (sbitmap concerns)

The function name should probably have some combination of the strings 
emit_ and _at or _into to make it clear what it's doing. This and the 
following function have some logical operations on the bitmaps which are 
not explained anywhere. In general a much better overview of the 
intended operation of this pass is needed.

> +	{
> +	  bitmap_and_compl (epi, SW (e->src)->has_concern,
> +			    SW (e->dest)->has_concern);
> +	  bitmap_and_compl (pro, SW (e->dest)->has_concern,
> +			    SW (e->src)->has_concern);
> +	  bitmap_and (epi, epi, concerns);
> +	  bitmap_and (pro, pro, concerns);
> +	  bitmap_and_compl (epi, epi, SW (e->dest)->head_concern);
> +	  bitmap_and_compl (pro, pro, SW (e->dest)->head_concern);
> +	  bitmap_and_compl (epi, epi, SW (e->src)->tail_concern);
> +	  bitmap_and_compl (pro, pro, SW (e->src)->tail_concern);

Likewise here.


Bernd