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

On 08/27/13 16:27, Iyer, Balaji V wrote:
Hello Aldy, I went through all the emails and here are the major
issues that I could gather (other than lowering the keywords after
gimplification, which I am skipping since it is more of an
optimization for now).

Ok, for now I am fine with delaying handling all this as a gimple tuple since most of your code lives in it's only little world :). But I will go on record saying that part of the reason that you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is because you don't have easy gimplified code to examine. Anyways, agreed, you can do this later.

1. Calling the gimplify_cilk_spawn on top of the gimplify_expr before
the switch-statement could slow the compiler down 2. I need a
CILK_SPAWN_STMT case in the switch statement in gimplify_expr (). 3.
No test for catching the suspicious spawned function warning 4.
Reasoning for expanding the 2 builtin functions in builtins.c instead
of just inserting the appropriate expanded-code when I am inserting
the function call.

Did I miss anything else (or misunderstand anything you pointed

Here are my answers to those questions above and am attaching a fixed
patch with the changelog entries:

1 & 2(partial): There are 3 places where we could have _Cilk_spawn:
both gimplified using gimplify_modify_expr. I have moved the
cilk_detect_spawn into this function. We will go into the
cilk_detect_spawn if cilk plus is enabled, and if there is a
cilk_frame (meaning the function has a Cilk_spawn in it) thereby
reducing the number of hits into this function significantly. Inside
this function, it will go into the function that has a spawned
function call and then unwrap the CILK_SPAWN_STMT wrapper and returns
true. This shouldn't cause a huge compilation time hit. 2. To handle
CALL_EXPR (e.g. _Cilk_spawn foo (x), where foo returns a void or the
return value of it is ignored), I have added a CILK_SPAWN_STMT case.
Again, I am calling the detect_cilk_spawn and we will only step into
this function if Cilk Plus is enabled and if there is a cilk-frame
(i.e saying the function has a cilk spawn in it). If there is an
error (seen_error () == true), then it just falls through into
CALL_EXPR and is handled like a normal call expr not spawned
expression. 3. This warning rarely get hit, and I have seen it hit

See my comments below on this.

only when you are spawning a constructor in C++. To my knowledge, we
have not had anyone report that they have hit this warning. I just

Ok, leave the warning, but do include a test case.

kept it in there just in case as a heads-up. 4. The reason why I am
handling pop_frame and detach functions in builtins.c is because one
of the things that LTO does is to remove the frame pointer. All Cilk
functions must use the frame pointer. When LTO is invoked, it is hard
to see what function is a cilk function and what is not. The CFUN
flag that says it is a cilk function gets cleared out. But, the
builtin functions are expanded during LTO phase and I set the
is_cilk_function flag when it is expanding pop_frame and detach. The
other option that I was thinking was to have frame pointer on when
Cilk Plus is enabled, but this is a bit over-kill and can cause
performance issues.

I think the reason you have to do all these gymnastics is bececause you are waiting so late to expand. You could expand into trees and not have to worry about frame pointers and such. See fold_builtin_* in builtins.c. *However*, if you think you can generate better code by delaying expansion all the way until RTL, then I'm fine with your current approach.

Also, I had added a couple more tests to catch a couple cases.

+  /* Implicit _Cilk_sync must be inserted right before any return statement
+     if there is a _Cilk_spawn in the function (checked by seeing if
+     cilk_frame_decl is not NULL_TREE).  If the user has provided a
+     _Cilk_sync, the optimizer should remove this duplicate one.  */
+  if (flag_enable_cilkplus && cfun->cilk_frame_decl != NULL_TREE)

Again, never document the obvious things your code is doing. For example, you can remove "(checked by seeing if > + cilk_frame_decl is not NULL_TREE)". It's obvious by looking at the code.

+      /* If there are errors, there is no point in expanding the
+         _Cilk_spawn.  Just gimplify like a normal CALL_EXPR.  */
+      && !seen_error ())

Similarly here.  No need to document the obvious.

+      /* If there are errors, there is no point in expanding the
+         _Cilk_spawn.  Just gimplify like a normal MODIFY or INIT_EXPR.  */
+      && !seen_error ())

And here.

Alos, if the canonical way of checking if a function has a cilk_spawn in it is always "cfun->cilk_frame_decl != NULL_TREE", then you should abstract this into an inline function. The implementation will be trivial to change if we ever decide to keep that information elsewhere. Perhaps something like:

static bool inline
cilk_function_has_spawn (struct function *func)
  return func->cilk_frame_decl != NULL_TREE;

Do all these hooks you have in lang_hooks_for_cilkplus really have to be hooks? That is, do you need different variants of them for C and for C++, or can you make do with one? Because if you only need one, there's no need for a hook.

For example:

+gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p
+  tree sync_expr = expand_cilk_sync ();
+  *expr_p = NULL_TREE;
+  gimplify_and_add (sync_expr, pre_p);
+  return GS_ALL_DONE;

The above is a hook. Will there be a different version of this for C++? If not, just call it directly. Similarly for gimplify_cilk_spawn(), and the rest of the hooks.

+/* Returns true if *EXP0 is a recognized form of spawn.  Recognized forms are,
+   after conversion to void, a call expression at outer level or an assignment
+   at outer level with the right hand side being a spawned call.
+   Note that `=' in C++ may turn into a CALL_EXPR rather than a MODIFY_EXPR.  */
+cilk_detect_spawn_in_expr (tree *exp0)

I don't like the name of this function or the corresponding hook (cilk_detect_spawn). You are actually detecting whether the expression contains a spawn *and* unwrapping it. So its use hides the fact that you are also modifying the expression in place.

+  if (flag_enable_cilkplus && cfun->cilk_frame_decl != NULL_TREE
+      && lang_hooks.cilkplus.cilk_detect_spawn (expr_p)
+      /* If there are errors, there is no point in expanding the
+         _Cilk_spawn.  Just gimplify like a normal CALL_EXPR.  */
+      && !seen_error ())
+    return (enum gimplify_status)
+      lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p, NULL);

I would much rather have cilk_detect_spawn_and_unwrap() or something. Not pretty, but at least it doesn't hide what it's doing. That is, it's not just a check... it can transform the expression.

Also, please document the fact that you are unwrapping and changing the expression in the comment to cilk_detect_spawn_in_expr.

+	  gcc_assert (flag_enable_cilkplus
+		      && cfun->cilk_frame_decl != NULL_TREE
+		      && lang_hooks.cilkplus.cilk_detect_spawn (expr_p));
+	  if (!seen_error ())
+	    {
+	      ret = (enum gimplify_status)
+		lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
+							 post_p);
+	      break;
+	    }

Remove the check for flag_enable_cilkplus. No one really builds that but cilk plus, and if for some reason it gets generated, it's pretty easy to debug/see why. See how we handled a slew of other front-end specific TREEs (like TRANSACTION_EXPR). No one ever checks for flag_*.

But BTW, your changes to gimplify.c are much better. It's a much cleaner approach. Thanks.

+	  {
+	    if (!cfun->cilk_frame_decl)
+	      {
+		error_at (input_location, "expected %<_Cilk_spawn%> before "
+			  "%<_Cilk_sync%>");
+		ret = GS_ERROR;
+	      }

First, surely you have a location you can use, instead of the generic input_location (perhaps the location for the CILK_SYNC_STMT??). Also, Can you not check for this in c_finish_cilk_sync_stmt(), or the corresponding code-- that is, in the FE somewhere? And hopefully, in a place you can share with the C++ FE? If it is a real pain, I am willing to let this go, since it happens only in the Cilk code path, though the general trend (especially with Andrew's proposed changes) is to do all type checking as close to the source as possible.


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