This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]