This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

On 10/16/13 15:49, Iyer, Balaji V wrote:
In ira.c:

+       /* 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.
Nevermind -- that seems to be the location where this is detected now. So this is fine.

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.
Thanks. Presumably the comment "We don't want the private variables anymore" is the relevant code/comment?

Does anything actually ensure we don't have multiple syncs?

Well, _Cilk_sync expands to something like this:

If (!sync_occurred)

So, having multiple Cilk syncs doesn't harm, just that the then case of the if-statement will not be taken.
OK.  Thanks.

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:
+         gcc_assert
+           (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,
+                                                        post_p);
+             break;
+           }
+         /* 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.

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.
Figured it was ugly at best to avoid consuming the cilk tokens when not in cilk mode.

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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]