From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8014 invoked by alias); 15 Jul 2014 16:10:31 -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 7988 invoked by uid 89); 15 Jul 2014 16:10:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: out3-smtp.messagingengine.com Received: from out3-smtp.messagingengine.com (HELO out3-smtp.messagingengine.com) (66.111.4.27) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 15 Jul 2014 16:10:27 +0000 Received: from compute6.internal (compute6.nyi.mail.srv.osa [10.202.2.46]) by gateway1.nyi.mail.srv.osa (Postfix) with ESMTP id 61FA22148D for ; Tue, 15 Jul 2014 12:10:13 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute6.internal (MEProxy); Tue, 15 Jul 2014 12:10:13 -0400 Received: from [129.199.97.210] (unknown [129.199.97.210]) by mail.messagingengine.com (Postfix) with ESMTPA id D0B9AC0000C; Tue, 15 Jul 2014 12:10:12 -0400 (EDT) Message-ID: <53C55264.7070404@grosser.es> Date: Tue, 15 Jul 2014 16:15: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: Roman Gareev CC: Mircea Namolaru , gcc-patches@gcc.gnu.org Subject: Re: [GSoC] generation of Gimple code from isl_ast_node_user References: <53C27B10.6050703@grosser.es> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2014-07/txt/msg01078.txt.bz2 On 15/07/2014 16:59, Roman Gareev wrote: >> >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. OK. Then I miscounted. Please commit this as a separate (obvious) change and post the ChangeLog to gcc-patches. We should keep unrelated cleanup patches out of patch reviews. >> >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 just looked again into chrec_apply_map and it requires a vector that maps from the loop id to a tree. So the existing interface requires this kind of input. I think the interface is not nice, but don't think we should worry about this at the moment. Maybe add a FIXME that says: FIXME: Instead of using a vec that maps each loop id to a possible chrec, we could consider using a map that maps loop ids to the corresponding tree expressions. >> >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.). I see. Could you use vec_safe_grow_cleared(iv_map, loop_num) instead? This shows probably better that you zero initialize the vector. >> >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? Right. You can just start by adjusting your test case, naming it isl-ast-gen-single-loop.c and adding the following at the beginnign of the file: /* { dg-do run } */ /* { dg-options "-O2 -fgraphite-identity" } */ Some more comments: > /* We always use signed 128, until isl is able to give information about > types */ > > -static tree *graphite_expression_size_type = &int128_integer_type_node; > +static tree *graphite_expression_size_type = int128_integer_type_node ? > + &int128_integer_type_node : > + &long_long_integer_type_node; Please keep unrelated changes out of the patch review. > /* Converts a GMP constant VAL to a tree and returns it. */ > > @@ -460,7 +464,8 @@ > case isl_ast_op_lt: > { > // (iterator < ub) => (iterator <= ub - 1) > - isl_val *one = isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond), 1); > + isl_val *one = > + isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond), 1); Please keep unrelated changes out of the patch review. Also, I think the patch now got nice and clean. Tobias