Re: [GSoC] generation of Gimple code from isl_ast_node_user

[Tobias Grosser <tobias at grosser dot es>]

On 15/07/2014 16:59, Roman Gareev wrote:
>> >This is a pure style change which seems unrelated. Also, is the 
original
> > >line really too long? I may have miscounted, but it seems to fit
> > >exactly.
> If I am not mistaken, lines should be limited to 80 characters,
> according to conventions, which are mentioned here
> https://gcc.gnu.org/wiki/CppConventions. This line contains 81
> characters.

OK. Then I miscounted. Please commit this as a separate (obvious) change
and post the ChangeLog to gcc-patches.

We should keep unrelated cleanup patches out of patch reviews.

> > >In polly we just create an iv_map from [0, max-loop-depth-within-scop], so
> > >it contains at most as many elements as the maximal loop depth. Your map
> > >is unnecessarily large, as it contains all loops in the function.
> > >
> > >If we can avoid this immediately, e.g. by indexing according to the
> > >loop-depths that would be great. On the other side, if the existing
> > >functions require this, I propose to not touch this area, but to add a
> > >fixme that documents this issue. We can solve it after having removed
> > >the CLooG codegen.
> If I am not mistaken, the existing function doesn't require that
> iv_map contains at most as many elements as the maximal loop depth.
> (If we consider chrec_apply_map (I think that it is the only function,
> which works with iv_map), we'll see that it calls chrec_apply when the
> expression is not NULL. In our case, we push NULL_TREEs into iv_map to
> extend its size to the maximal loop depth. They aren't considered,
> because tree NULL_TREE is equivalent to NULL, according to tree.h)
>
> Maybe we could use a number of the innermost loop that contains the
> basic block gbb. If I am not mistaken, outer loops considered in
> build_iv_mapping have smaller numbers than it. What do you think about
> this?

I just looked again into chrec_apply_map and it requires a vector that
maps from the loop id to a tree. So the existing interface requires this
kind of input. I think the interface is not nice, but don't think we
should worry about this at the moment.

Maybe add a FIXME that says:

FIXME: Instead of using a vec<tree> that maps each loop id to a possible
chrec, we could consider using a map<int, tree> that maps loop ids
to the corresponding tree expressions.

> > >Why do you need to first push NULL_TREEs into this vec, which will be
> > >overwritten right after?
> In the current implementation, we'll get the error “internal compiler
> error: in operator[], at vec.h:736”, if we try to assign a value to
> iv_map via [] and remove initial pushing of NULL_TREEs. We could push
> new values into iv_map in build_iv_mapping, but I am not sure if there
> are cases, which require special processing (for example, NULL_TREEs
> should be pushed into iv_map before the next old_loop->num. Possibly,
> there are no such cases.).

I see. Could you use vec_safe_grow_cleared(iv_map, loop_num) instead?
This shows probably better that you zero initialize the vector.

> > >It is unclear how this patches have been tested. Can you elaborate?
> I've compared the result of Gimple code generation (I've attached them
> to the letter) for the following examples:
>
> int
> main (int n, int *a)
> {
>    int i;
>
>    for (i = n; i < 100; i++)
>      a[i] = i;
>
>   return 0;
> }
>
>
> int
> main (int 0, int *a)
> {
>    int i;
>
>    for (i = n; i < 100; i++)
>      a[i] = i;
>
>   return 0;
> }
>
> > >Also, we need to find a way to test this in gcc itself, possibly by
> > >running test cases that already work with both the cloog and the isl ast
> > >generator.
> Maybe we could choose the strategy, which was used in
> /gcc/testsuite/gcc.dg/graphite/interchange-1.c,
> /gcc/testsuite/gcc.dg/graphite/interchange-mvt.c,
> /gcc/testsuite/gcc.dg/graphite/run-id-5.c?
> (The result of the transformed function is compared to the assumed
> value and the abort function is called in case of inequality.) What do
> you think about this?

Right. You can just start by adjusting your test case, naming it
isl-ast-gen-single-loop.c

and adding the following at the beginnign of the file:

/* { dg-do run } */
/* { dg-options "-O2 -fgraphite-identity" } */

Some more comments:

>   /* We always use signed 128, until isl is able to give information about
>   types  */
>
> -static tree *graphite_expression_size_type = &int128_integer_type_node;
> +static tree *graphite_expression_size_type = int128_integer_type_node ?
> +					     &int128_integer_type_node :
> +					     &long_long_integer_type_node;

Please keep unrelated changes out of the patch review.

>   /* Converts a GMP constant VAL to a tree and returns it.  */
>
> @@ -460,7 +464,8 @@
>       case isl_ast_op_lt:
>         {
>           // (iterator < ub) => (iterator <= ub - 1)
> -        isl_val *one = isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond), 1);
> +        isl_val *one =
> +          isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond), 1);
Please keep unrelated changes out of the patch review.

Also, I think the patch now got nice and clean.

Tobias