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

[Aldy Hernandez <aldyh at redhat dot com>]

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
> out)?
>
> 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:
> INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and MODIFY_EXPRS are
> 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:

> +int
> +gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p
> +		    ATTRIBUTE_UNUSED)
> +{
> +  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.  */
> +
> +bool
> +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.

> +	case CILK_SPAWN_STMT:
> +	  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.

> +	case CILK_SYNC_STMT:
> +	  {
> +	    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.

Aldy