Re: [patch] Lno branch merge part 5 -- #s of loop iterations

[Mark Mitchell <mark at codesourcery dot com>]

Zdenek Dvorak wrote:

> Hello,
>
> this patch contains some functions dealing with determining numbers of
> iteration of a loop.  To break a circular dependency on the scalar
> evolutions analysis, dummy functions (simple_iv and
> instantiate_parameters) are provided.
>  
This looks pretty good to me.  I don't think I should approve it without 
letting RTH comment, since he's been looking at the previous patches in 
this series, but the structure makes sense to me.   However, if you 
don't hear back from him in a bit, let me know, and I'll see if I can help.

I have indicated a few changes below, mostly to help with documentation 
issues; these are things that made it harder for me to understand your 
change.  Please make these changes before check-in.

> + /* In tree-ssa-loop*.c  */
>  
This comment should go on the functions, not the structure definition, 
since the structure definition is not in the source files.

> + /* Description of number of iterations of a loop.  */
> + struct tree_niter_desc
> + {
> +   tree assumptions;	/* Assumptions for the number of iterations be valid.  */
> +   tree may_be_zero;	/* Condition under that the loop exits in the first
> + 			   iteration.  */
> +   tree niter;		/* Number of iterations.  */
> +   tree additional_info;	/* Additional conditions taken into account when
> + 			   deriving the information above.  */
>  
For each of these fields, it would be better to give more details. 

For example, for "assumptions", I would say something like:

"A boolean-valued expression.  If this expression evalutes to false 
right before the first iteration of the loop, then the other fields in 
this structure should not be used; there is no guarantee that they will 
be correct."

> + /* Checks whether ARG is either NULL_TREE or constant zero.  */
>  
For accuracy, this should read "Returns true iff ARG is either NULL_TREE 
or a constant zero."   Just saying "Checks" does not indicate what value 
is returned in what case

> + /* Computes inverse of X modulo 2^s, where MASK = 2^s-1.  */
>  
Likewise, say "Returns" not "Computes".  I'll let you look for other 
places that might have a similar issue.

> +   /* We can take care of the case of two induction variables chasing each other
> +      if the test is NE. I have never seen a loop using it, but still it is
> +      cool.  */
>  
Let's avoid first-person singular in comments.  It leads future readers 
to wonder who "I" is.  Also, the editorial commentary about the coolness 
of this feature should be removed. :-)  Also, what does "chasing each 
other" mean?  It might help to put an example in the code.

> +       /* We assume pointer arithmetics never overflows.  */
>  
Grammatically, this should be either "pointer arithmetic" or "pointer 
operations".

> + /* Tries to simplify EXPR using the evolutions of the loop invariants
> +    in the outer loops.  */
>  
You should say what the "loop" parameter is here, and what is returned.

> + /* Stores description of number of iterations of LOOP derived from EXIT
> +    in NITER.  */
>  
And return what?

> +   niter->assumptions = convert (boolean_type_node, integer_zero_node);
>  
Is there a reason not to use boolean_false_node here?

> + #define MAX_ITERATIONS_TO_TRACK 1000
>  
All such values should go in params.h.

--
Mark Mitchell
CodeSourcery, LLC
(916) 791-8304
mark@codesourcery.com