Re: [Graphite-Branch] Skeleton for new SCoP detection.

[Sebastian Pop <sebpop at gmail dot com>]

On Wed, Jun 2, 2010 at 14:39, Tobias Grosser <grosser@fim.uni-passau.de> wrote:
> Hi,
>
> I would like to start the development of a region based SCoP detection in
> Graphite. As discussed in the Graphite phone call today, I submit the first
> patch for review.
>
> This patch adds the skeleton of the new SCoP detection. It will be executed in
> parallel to the old one SCoP detection to gain test coverage.
>
> 2010-06-02 ?Tobias Grosser ?<grosser@fim.uni-passau.de>
>
> ? ? ? ?* graphite-scop-detection.c (is_scop_p): New.
> ? ? ? ?(build_scops_new): New. A skeleton for the new scop detection.
> ? ? ? ?(build_scops_old): Renamed from build_scops.
> ? ? ? ?(build_scops): New version. Call the new and the old scop
> ? ? ? ?detection.
>
> The patch survived C/C++/Fortran bootstrap on Linux 64bit.
>
> OK for graphite branch for further testing?

Ok with a few corrections, see below:

> +static bool
> +is_scop_p (ATTRIBUTE_UNUSED refined_region_p region)

The ATTRIBUTE_UNUSED is generally put after the name of the parameter,
like this:

is_scop_p (refined_region_p region ATTRIBUTE_UNUSED)

> ?{
> - ?struct loop *loop = current_loops->tree_root;
> - ?VEC (sd_region, heap) *regions = VEC_alloc (sd_region, heap, 3);
> + ?/* TODO: Are there any harmful bbs in the region? ?*/
> + ?/* TODO: Have all loops a # of iterations that can be expressed

Please use full text "number of iterations" and avoid the "#" abbreviation.

> + ? ? ? ? ?by an affine linear function. ?*/
> + ?/* TODO: Is there only well structured control flow in the region?
> + ? ? ? ? ?* All loops have just one exit?
> + ? ? ? ? ?* All loops are detected by gcc's loop detection?
> + ? ? ? ? ?* All conditions are well nested? ?*/
> +
> + ?return false;
> +}
> +
> +/* Find Static Control Parts (SCoP) in the current function.
> + ? (New version using a more structured approach) ?*/

Please remove the parentheses and the text within, as this code will
replace the older code at one point and we will forget to remove the
comment.  You can say:

/* Find in a structured way Static Control Parts (SCoP) in the current
   function.  */

> - ?/* Run new scop detection in parallel. ?*/
> +static void
> +build_scops_new (void)
> +{
> +
> + ?VEC (refined_region_p, heap) *scops = VEC_alloc (refined_region_p, heap, 3);
> + ?VEC (refined_region_p, heap) *check = VEC_alloc (refined_region_p, heap, 3);
> +
> + ?/* TODO: Do we need to call canonicalize_loop_closed_ssa_form()? ?*/
> +

I think so, as the code generation expects to see only the canonical
form of the loop closed SSA.

> + ?/* Build new region tree. ?*/
> ? refined_region_p new_region = calculate_region_tree ();
> +
> + ?/* Find the maximal valid regions. ?*/
> + ?VEC_safe_push (refined_region_p, heap, check, new_region);
> +
> + ?while (VEC_length (refined_region_p, check) != 0)
> + ? ?{
> + ? ? ?refined_region_p region = VEC_last (refined_region_p, check);
> + ? ? ?VEC_pop (refined_region_p, check);
> +
> + ? ? ?if (is_scop_p (region))
> + ? ? ? VEC_safe_push (refined_region_p, heap, scops, region);
> + ? ? ?else
> + ? ? ? {
> + ? ? ? ? int ix;
> + ? ? ? ? refined_region_p subregion;
> +
> + ? ? ? ? for (ix = 0;
> + ? ? ? ? ? ? ?VEC_iterate (refined_region_p, region->children, ix, subregion);
> + ? ? ? ? ? ? ?ix++)
> + ? ? ? ? ? {
> + ? ? ? ? ? ? VEC_safe_push (refined_region_p, heap, check, subregion);
> + ? ? ? ? ? }

You can remove the {} around the single statement of the loop.