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
