This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
- From: Jeff Law <law at redhat dot com>
- To: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>, "rth at redhat dot com" <rth at redhat dot com>, "Jason Merrill (jason at redhat dot com)" <jason at redhat dot com>, "Aldy Hernandez (aldyh at redhat dot com)" <aldyh at redhat dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 22 Oct 2013 23:11:41 -0600
- Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
- Authentication-results: sourceware.org; auth=none
- References: <BF230D13CA30DD48930C31D4099330003A46B6DF at FMSMSX101 dot amr dot corp dot intel dot com> <525F03CE dot 9000801 at redhat dot com> <BF230D13CA30DD48930C31D4099330003A488BAF at FMSMSX101 dot amr dot corp dot intel dot com>
On 10/16/13 15:49, Iyer, Balaji V wrote:
Nevermind -- that seems to be the location where this is detected now.
So this is fine.
+ /* We need a frame pointer for all Cilk Plus functions that use
+ Cilk keywords. */
+ || (flag_enable_cilkplus && cfun->is_cilk_function)
Can you explain to me a bit more why you need a frame pointer? I'm trying to
determine if it's best to leave this as-is or have this code detect a property in the
generated code for the function. From a modularity standpoint it seems pretty
gross that we have to peek at this within IRA.
Cilk Runtime functions changes the stack pointer. So, frame pointer is necessary.
Thanks. Presumably the comment "We don't want the private variables
anymore" is the relevant code/comment?
In a couple places I saw this comment:
+ /* Cilk keywords currently need to replace some variables that
+ ordinary nested functions do not. */ bool remap_var_for_cilk;
I didn't see anywhere that explained exactly why some variables that do not
ordinarily need replacing need replacing when cilk is enabled. If it's in the patch
somewhere, just point me to it. If not, add documentation about why these
variables need remapping for cilk.
It is used in the cilk_outline function.
Does anything actually ensure we don't have multiple syncs?
Well, _Cilk_sync expands to something like this:
So, having multiple Cilk syncs doesn't harm, just that the then case of the if-statement will not be taken.
What's the thinking behind parsing calls to cilk_spawn as a normal call if there's
an error? Referring to this code in gimplify.c:
+ case CILK_SPAWN_STMT:
+ (fn_contains_cilk_spawn_p (cfun)
+ && lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
+ if (!seen_error ())
+ ret = (enum gimplify_status)
+ lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
+ /* If errors are seen, then just process it as a CALL_EXPR.
Well, if there is an error the compiler is not going to produce an executable. So, I just let the compiler go far as it can and catch all the other errors. If the error is cilk related, we have already called them out on it. Adding _Cilk_spawn specific routines would add additional complication.
I guess that's a reasonable fallback position in case of an error.
Figured it was ugly at best to avoid consuming the cilk tokens when not
in cilk mode.
Meta-question, when we're not in cilk mode, should we be consuming the cilk
tokens? I'm not familiar at all with our parser, so I'm not sure if we can handle
this gracefully. Though I guess parsing hte token and warning/error if not in Cilk
mode is probably the best course of action.
In the compiler, I couldn't make conditional tokens. When the parser hits a _Cilk_spawn or _Cilk_sync token, it will check if Cilk Plus is enabled or will complain. Now that I think about it in detail, I suppose it will also block if anyone wants to have a variable name called _Cilk_spawn or _Cilk_sync and not using -fcilkplus. But, they start with '_', and so I guess it is not a normal case.
Can you take a look at calls.c::special_function_p and determine if we need to
do something special for spawn here?
I will look into it and let you know.
Any word on this?