> No, no idea. To my understanding the entry block should not even appear
> within a scop (see build_scops, where we only start detecting scops at
> the successor of the entry_block). Maybe we replace this with an assert
> to get a good error message in case I have missed something.

Yes, I think this would be a good solution at this moment.

> I think this region is actually not needed. At the place where you need
> it, you have a pbb available, from which you can obtain a pointer to the
> surrounding scop and from this you can obtain this region itself.

Yes, this is redundant.

> 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.

> 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 was just briefly looking the code to remind me what this
> bb_pbb_mapping hash table is about, but could not find the reason it is
> needed. Do you know why it is needed?
>
> Is it necessary for this patch? Or did you just copy it from the
> previous code generation?

Yes, I've forgotten to remove this. If I am not mistaken,
bb_pbb_mapping is used for checking for the loop parallelism in
graphite-clast-to-gimple.c.

> 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.).

> 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?

--
                                   Cheers, Roman Gareev.