Re: [Graphite] Add analysis pass to build the refined program structure tree.

[Tobias Grosser <grosser at fim dot uni-passau dot de>]

On 05/27/10 17:06, Sebastian Pop wrote:
> On Thu, May 27, 2010 at 01:43, Tobias Grosser<grosser@fim.uni-passau.de>  wrote:
> > Here an updated version.
>
> +#ifdef HAVE_cloog
> +#include "refined-regions.h"
>
> Why do we have all this code conditionally compiled on HAVE_cloog?
Removed.

> +/* Check is BB is element of REGION.  */
>
> This should be /* Check that BB is an element of REGION.  */
>
> +/* Check is the INNER regions is contained in the OUTER region.  */
> +/* Check if BB is part of the dominance frontier of ENTRY, because it was
> +/* Check if ENTRY and EXIT form a refined region.  DFS contains the dominance
> +/* Check if two bb to bb objects BB_BB1 and BB_BB2 are equal.  */
>
> Please s/if/that/.
>
> +  /* A array that contains for each basic block its dominance frontier.  */
>
> s/A array/An array/
>
> +/* Allocate a new bb to bb object mapping from KEY to VALUE.  */
> +/* Calculate the hash code of the bb to bb object BB_BB.  */
> +/* Check if two bb to bb objects BB_BB1 and BB_BB2 are equal.  */
>
> Please use some quotes around "bb to bb" or use bb-to-bb or even the
> functional style notation bb->bb, as otherwise the sentence is
> difficult to parse ;-)
>
> +/* Insert an object mapping from BB to REG into REGMAP. Overwrite
> +   the old value if the BB already exists.  */
>
> Dot space space.
>
> +static void find_regions (bitmap *dfs, htab_t bbmap)
>
> Please put the return type of functions on a separate line.
>
> +/* Free REGION and all its subregions.  */
> +void
> +free_region_tree (refined_region_p region)
>
> Empty line between comment and function def.
>
> +static refined_region_p
> +create_region (basic_block entry, basic_block exit, refined_region_p parent,
> +htab_t bbmap)
>
> Please align under the open brace.
Done.

> Could you rename bb_in_ref_region into bb_in_region, to be consistent
> with the other names like region_in_region, create_region.

No. There exists already a bb_in_region in sese.h and a bb_in_region_p 
somewhere else in gcc. To stay consistent I renamed all public functions 
to refined_region_* .
The current scop detection is the only user of bb_in_region at the 
moment, so hopefully we can get back to use plain region_* names soon.
I did not rename the private functions in the .c file. Is this OK?

Another change I did:

Before there was this comment in the code:
+  /* XXX: How do I release/free the dominance information.  */

Now I check if the dominance information was available before my 
analysis. If it was available, I will not do anything. Otherwise I will
free the dominance information.

Thanks for your helpful comments.

Tobi

Attachment: 0001-Add-analysis-pass-to-build-the-refined-program-struc_v3.patch
Description: Text document