[patch] Autoparallelization

Diego Novillo dnovillo@google.com
Thu Sep 13 18:50:00 GMT 2007


On 9-Sep-07, at 09:47 , Zdenek Dvorak wrote:

> + @item -ftree-parallelize-loops=n
> + @opindex ftree-parallelize-loops=n
> + Parallelize loops, i.e., split their iteration space to run in n  
> threads.
> +

Add a sentence or two describing how the cost model works and what  
types of loops are expected to be parallelized.

> + /* Parallelization.  */
> +
> + static bool
> + gate_tree_parallelize_loops (void)
>

> + static unsigned
> + tree_parallelize_loops (void)
>

> + struct tree_opt_pass pass_parallelize_loops =

Why not declare these three in tree-parloops.c?  Not that it matters  
much, just curious.

> +
> +   /* The iterations of the loop may comunicate only through bivs  
> whose

s/comunicate/communicate/

> + 	{
> + 	  if (dump_file && (dump_flags & TDF_DETAILS))
> + 	    fprintf (dump_file, "  FAILED: non-biv phi node\n");

Hmm, these messages are typically read by users who are looking to  
parallelize their code.  The phrase 'non-biv phi node' will be  
perplexing to anyone not thoroughly familiar with compiler lingo.

> +
> +   /* Check for problems with dependences.  If the loop can be  
> reversed,
> +      the iterations are indepent.  */

s/indepent/independent/

> +   else if (dump_file && (dump_flags & TDF_DETAILS))
> +     fprintf (dump_file, "  FAILED: dependence check failed\n");

Similarly, how about something like "data dependencies exist across  
iterations" or "iterations are not independent of each other".

> + /* Moves the references to local variables in STMT from LOOP.   
> DECL_ADDRESS

s/from/out of/

> +    contains addresses for the references for that we have already  
> taken
> +    them.  */

Not sure what you wanted to say here.  Perhaps, "contains addresses  
of the references that had their address taken already"?

> + /* Moves all the variables used in LOOP and defined outside of it  
> (including
> +    the initial values of loop phi nodes, and *PER_THREAD if it is  
> a ssa
> +    name) to a structure created for this purpose.  The code
> +
> +    while (1)
> +      {
> +        use (a);
> +        use (b);
> +      }
> +
> +    is transformed this way:
> +
> + bb0:
> +    old.a = a;
> +    old.b = b;
> +
> + bb1:
> +    a' = new->a;
> +    b' = new->b;
> +    while (1)
> +      {
> +        use (a');
> +        use (b');
> +      }

Watch indentation here.

>
> +       /* We should mark *arg_struct call clobbered.  However,  
> this means
> + 	 we would need to update virtual operands for every function call.
> + 	 To avoid this, we pretend this variable is volatile.  */
> +       TREE_THIS_VOLATILE (*arg_struct) = 1;
>
Hmm, OK I guess.  Not too happy about this, but I understand why  
you'd want to do this.

> +
> + /* Creates and returns an empty function to that the body of the  
> loop
> +    is later split.  */
>
Not sure what you're trying to say here.  Maybe "... an empty  
function that will receive the body of a parallelized loop"?


> +   /* Emit OMP_FOR.  */
> +   TREE_OPERAND (cond, 0) = cvar_base;
> +   type = TREE_TYPE (cvar);
> +   t = build_omp_clause (OMP_CLAUSE_SCHEDULE);
> +   OMP_CLAUSE_SCHEDULE_KIND (t) = OMP_CLAUSE_SCHEDULE_STATIC;

Do we want to specify this as a --param?  I can see future  
improvements where we allow the user to determine what scheduling to  
use and whatnot.  The patch looks like it would support different  
loop schedulings, or is there something I'm missing?


The rest looks fine.  Thanks for this feature!



More information about the Gcc-patches mailing list