This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
- From: Aldy Hernandez <aldyh at redhat dot com>
- To: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>
- Cc: "rth at redhat dot com" <rth at redhat dot com>, Jeff Law <law at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>
- Date: Tue, 20 Aug 2013 17:04:10 -0500
- Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
- References: <BF230D13CA30DD48930C31D4099330003A455EF0 at FMSMSX101 dot amr dot corp dot intel dot com> <52012923 dot 6030409 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A457F85 at FMSMSX101 dot amr dot corp dot intel dot com> <5204C718 dot 9050402 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A458F86 at FMSMSX101 dot amr dot corp dot intel dot com>
[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.