Re: [GSoC] Addition of ISL AST generation to Graphite

[Tobias Grosser <tobias@grosser.es>]

Thanks Sebastian for the review! It is good to see you again on the 
mailing list!

On 23/06/2014 11:29, Sebastian Pop wrote:
> Please add a FIXME note in graphite_regenerate_ast_isl saying that
> this is not yet a full implementation of the code generator with ISL
> ASTs.
> It would be useful to make the current graphite_regenerate_ast_isl
> working by calling graphite_regenerate_ast_cloog and adding the fixme
> note above saying that we rely on the cloog code generator until we
> implement the ISL AST parsing.

I would prefer to avoid this for the following two reasons:

1) Having a clear internal compiler error on unimplemented features
    allows us to easily understand which parts are working and which
    ones are not.

2) Going forward we will implement the isl ast generation step by step.
    To my understanding there is no reasonable way to do parts of the
    ast generation with isl and parts with CLooG. Developing such a
    hybrid generation seems not useful at all.

Instead of developing the ast code generation step-by-step in the 
graphite source tree, we could develop it outside of gcc and only commit 
a single large patch performing the switch. I personally
prefer the incremental development approach.

> There is also a minor code style issue in:
>
> isl_set * context_isl = isl_set_params (isl_set_copy (scop->context));
>
> please remove the space after *:
>
> isl_set *context_isl = isl_set_params (isl_set_copy (scop->context));
>
> Otherwise the two patches look good to me.

Cheers,
Tobias