Re: [graphite] SCoP detection update to fix Bug 37372 and various others

["Sebastian Pop" <sebpop at gmail dot com>]

> +/* A single entry single exit region, defined using bbs as borders.
> +   All controlflow touching this region, comes in passing ENTRY and

s/controlflow/control flow/

> +   leaves passing EXIT.  By using bbs instead of edges for the borders
> +   we are able to represent also SESE regions,  that do not have a single
> +   entry or exit edge.

If they do not have a single entry or single exit, then these regions
are not SESE regions:
s/represent also SESE regions,  that/represent also regions that/


> +typedef struct sd_region_p
> +{
> +  /* The entry bb dominates all bbs in the SESE region.  It is part of the

s/in the SESE region/in the region/

> +     region.  */
> +  basic_block entry;
> +
> +  /* The exit bb postdominates all bbs in the SESE region, BUT is NOT

s/in the SESE region/in the region/

Do not use capital letters for BUT and NOT.

> +     part of the region.  */
> +  basic_block exit;
> +} sd_region;


>  /* Checks, if a bb can be added to a SCoP.  */
>
>  static struct scopdet_info
> -scopdet_edge_info (edge ee,
> -		   VEC (scop_p, heap) **scops, gbb_type type, gimple *stmt)
> -	
> +scopdet_edge_info (basic_block bb, VEC (sd_region, heap) **scops, gbb_type type)

You should also rename this function into scopdet_bb_info now, and
please document the parameters.

> +  /* XXX: ENTRY_BLOCK_PTR could be optimized in later steps.  */
> +  stmt = harmful_stmt_in_bb (ENTRY_BLOCK_PTR, bb);

We could use, instead of ENTRY_BLOCK_PTR, the exit of the last closed
scop that has been copied in the scops vector, i.e. when
result.difficult was set.  You could store a pointer to that bb in a
static variable when you call move_sd_regions.

> +  return  dominated_by_p (CDI_DOMINATORS, bb, region->entry)

one space between return and dominated_by_p.

> +  gcc_assert(find_single_entry_edge (region));

one space between gcc_assert and open parenthesis.

> +  sd_region *r = (sd_region*) e->aux;

one space between sd_region and *.


> +  /* We create a forwarder bb (5) for all edges leaving this region
> +     (3->5, 4->5). All other edges leading to the same bb, are moved
> +     to a new bb (6). If these edges where part of another region (2->5)
> +     we update the region->exit pointer, of this region.

point space space new sentence.

s/where/were/

> +     Now there is only a single exit edge (5->6). */

point space space close comment.

The patch looks good, but somebody has to approve it.

Thanks,
Sebastian