From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17480 invoked by alias); 23 Jun 2014 09:35:57 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 17469 invoked by uid 89); 23 Jun 2014 09:35:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: out1-smtp.messagingengine.com Received: from out1-smtp.messagingengine.com (HELO out1-smtp.messagingengine.com) (66.111.4.25) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 23 Jun 2014 09:35:55 +0000 Received: from compute2.internal (compute2.nyi.mail.srv.osa [10.202.2.42]) by gateway1.nyi.mail.srv.osa (Postfix) with ESMTP id 4B36F20EC1 for ; Mon, 23 Jun 2014 05:35:53 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute2.internal (MEProxy); Mon, 23 Jun 2014 05:35:53 -0400 Received: from [192.168.1.106] (unknown [89.159.248.139]) by mail.messagingengine.com (Postfix) with ESMTPA id 5653CC0000C; Mon, 23 Jun 2014 05:35:51 -0400 (EDT) Message-ID: <53A7F4F6.6000707@grosser.es> Date: Mon, 23 Jun 2014 09:35:00 -0000 From: Tobias Grosser User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Sebastian Pop CC: Roman Gareev , GCC Patches , Mircea Namolaru Subject: Re: [GSoC] Addition of ISL AST generation to Graphite References: <53A1E2B9.3090205@grosser.es> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2014-06/txt/msg01741.txt.bz2 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