From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31415 invoked by alias); 20 Aug 2013 22:04:15 -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 31406 invoked by uid 89); 20 Aug 2013 22:04:14 -0000 X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 20 Aug 2013 22:04:13 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7KM4Cdb023895 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 20 Aug 2013 18:04:12 -0400 Received: from reynosa.quesejoda.com (vpn-57-142.rdu2.redhat.com [10.10.57.142]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7KM4BkL032260; Tue, 20 Aug 2013 18:04:11 -0400 Message-ID: <5213E7DA.2060505@redhat.com> Date: Tue, 20 Aug 2013 22:28:00 -0000 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: "Iyer, Balaji V" CC: "rth@redhat.com" , Jeff Law , "gcc-patches@gcc.gnu.org" , Jakub Jelinek Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C References: <52012923.6030409@redhat.com> <5204C718.9050402@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg01158.txt.bz2 [rth, law, jakub: Your input required throughout...please.] More review stuff... Overall, I must say, I'm not a big fan of the super early expansion you're doing right after parsing. I mean, you leave CILK_SPAWN and CILK_SYNC keywords as is (in tree form until gimplification) but there's this awful lot of expansion that goes on parallel to that. For instance, for this code: extern void testing(); extern void ending(); foo(){ _Cilk_spawn bar(); testing(); _Cilk_sync; ending(); } ...right after parsing you already generate: { __cilkrts_enter_frame_1 (&D.1776); try { _Cilk_spawn bar (); // keywords as trees, fine testing (); _Cilk_sync;; // keywords as trees, fine ending (); } finally // but all this try/finally // support stuff too early?? { D.1776.pedigree = D.1776.worker->pedigree; if ((D.1776.flags & 2) != 0) { __cilkrts_save_fp_ctrl_state (&D.1776); if (__builtin_setjmp (&D.1776.ctx) == 0) { __cilkrts_sync (&D.1776); } else { if ((D.1776.flags & 16) != 0) { __cilkrts_rethrow (&D.1776); } } } D.1776.worker->pedigree.rank = D.1776.worker->pedigree.rank + 1; D.1776.worker->current_stack_frame = D.1776.call_parent; __cilkrts_pop_frame (&D.1776); if (D.1776.flags != 16777216) { __cilkrts_leave_frame (&D.1776); } } } You seem to be hijacking finish_function(), to generate all this try/finally and builtin stuff here: > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > index f7ae648..ffd62c6 100644 > --- a/gcc/c/c-decl.c > +++ b/gcc/c/c-decl.c > @@ -8380,6 +8380,12 @@ finish_function (void) > /* Tie off the statement tree for this function. */ > DECL_SAVED_TREE (fndecl) = pop_stmt_list (DECL_SAVED_TREE (fndecl)); > > + /* IF the function has _Cilk_spawn in front of a function call inside it > + i.e. it is a spawning function, then add the appropriate Cilk plus > + functions inside. */ > + if (flag_enable_cilkplus && cfun->calls_cilk_spawn == 1) > + cfun->cilk_frame_decl = insert_cilk_frame (fndecl); I would've preferred to leave all expansion until *at least* gimplification. Although, looking at how OMP works, expansion happens even further down after the CFG has been built and EH has been handled. Seeing that _Cilk_spawn and _Cilk_sync heavily alter the flow (and possibly exceptions) of the program, I would guess that you need to follow a similar path with these two Cilk keywords. I don't think this is a blocker, especially since most everything seems to be contained in Cilk specific files, but I would like to get some feedback from Jeff/Richard/Jakub, and/or some other more globally savvy people :). Perhaps this could be done as a follow-up patch, if necessary. Having said that, at least expansion in finish_function() feels weird. > +tree > +c_build_cilk_spawn (location_t loc, tree call) > +{ > + if (!cilkplus_set_spawn_marker (loc, call)) > + return error_mark_node; > + tree spawn_stmt = build1 (CILK_SPAWN_STMT, TREE_TYPE (call), call); Do you need a TREE_TYPE here at all? Is it used anywhere? > +/* Marks CALL, a CALL_EXPR, as a spawned function call. */ > + > +tree > +c_build_cilk_spawn (location_t loc, tree call) > +{ > + if (!cilkplus_set_spawn_marker (loc, call)) > + return error_mark_node; Can you inline cilkplus_set_spawn_marker? It's not used anywhere else and it's relatively small. If you need it for C++, perhaps you can make c_build_cilk_spawn() generic enough to be used for both FE's? > +/* Helper function for walk_tree. If *TP is a CILK_SPAWN_STMT, then unwrap > + this "wrapper" and *WALK_SUBTREES is set to 0. The function returns > + NULL_TREE regardless. */ > + > +static tree > +unwrap_cilk_sync_stmt (tree *tp, int *walk_subtrees, void *) > +{ > + if (TREE_CODE (*tp) == CILK_SPAWN_STMT) > + { > + *tp = CILK_SPAWN_FN (*tp); > + *walk_subtrees = 0; > + } > + return NULL_TREE; > +} Why is this called unwrap_cilk_sync_stmt when it is unwrapping a spawn? > + case CILK_SYNC_STMT: > + pp_string (buffer, "_Cilk_sync;"); > + break; Drop the semi-colon. The end of dump_generic_node() should add the semi-colon if it's a statement. I bet you're probably getting two semi-colons at the end of _Cilk_sync in the dump. > +/* Returns a wrapper function for a _Cilk_spawn. */ > + > +static tree > +build_cilk_wrapper (tree exp, tree *args_out) This name is confusing. The world "build" is usually used to denote front-end building of trees, not gimplification. Seeing that build_cilk_wrapper() is only called from gimplify_cilk_spawn(), it's a bit confusing. > +/* This function will expand a cilk_sync call. */ > + > +static tree > +build_cilk_sync (void) > +{ > + tree frame = cfun->cilk_frame_decl; Similarly with this as well, which is mostly a helper for gimplify_cilk_sync(). And for that matter, it can be confused with c_build_cilk_sync() which does something completely different. I do see that you also call build_cilk_sync() from build_cilk_function_exit() here: > +/* This function will output the exit conditions for a spawn call. */ > + > +tree > +build_cilk_function_exit (tree frame, bool detaches, bool needs_sync) > +{ > + tree sync_expr = NULL_TREE; > + > + tree epi = alloc_stmt_list (); > + > + if (needs_sync) > + { > + sync_expr = build_cilk_sync (); > + append_to_statement_list (sync_expr, &epi); > + } It seems to me that you should have generated this implicit _Cilk_sync at function exit by an earlier pass. If jakub/rth/others think it is incorrect to expand so early (as I have pointed out), perhaps adding the _Cilk_sync could be done similarly to how OMP generates implicit OMP returns in the omplower stage. For example, for something like: foo(){ int i; #pragma omp for for (i=0; i < 10; ++i) bar(); } omplower expands to: #pragma omp for private(i) for (i = 0; i <= 9; i = i + 1) bar (); #pragma omp continue (i, i) #pragma omp return Anyways... let's wait to hear from the heavy hitters.