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

[Aldy Hernandez]

>>> --- gcc/expr.c
>>> +++ gcc/expr.c
>>> @@ -9569,6 +9569,21 @@ expand_expr_real_1 (tree exp, rtx target, enum
>> machine_mode tmode,
>>>   	}
>>>
>>>         return expand_constructor (exp, target, modifier, false);
>>> +    case INDIRECT_REF:
>>> +      {
>>> +	tree exp1 = TREE_OPERAND (exp, 0);
>>> +	if (modifier != EXPAND_WRITE)
>>> +	  {
>>> +	    tree t = fold_read_from_constant_string (exp);
>>> +	    if (t)
>>> +	      return expand_expr (t, target, tmode, modifier);
>>> +	  }
>>> +	op0 = expand_expr (exp1, NULL_RTX, VOIDmode, EXPAND_SUM);
>>> +	op0 = memory_address (mode, op0);
>>> +	temp = gen_rtx_MEM (mode, op0);
>>> +	set_mem_attributes (temp, exp, 0);
>>> +	return temp;
>>> +      }
>>
>> Ughhh, what's the rationale for this?  Are generic changes to
>> expand_expr really needed?
>
> Yes, I am expanding some variables of type "ptr->value" and those are emitted as INDIRECT_REF.

The fact that you are getting an INDIRECT_REF this late in the game is 
suspect.

Are you building with ENABLE_CHECKING, because it seems this should have 
been caught.  See the places in tree-cfg.c with this:

     case INDIRECT_REF:
       error ("INDIRECT_REF in gimple IL");
       return t;
>
>>
>>> +  /* During LTO, the is_cilk_function flag gets cleared.
>>> +     If __cilkrts_pop_frame is called, then this definitely must be a
>>> +     cilk function.  */
>>> +  if (cfun)
>>> +    cfun->is_cilk_function = 1;
>>
>> I don't know much about our LTO implementation, but perhaps you need to
>> teach LTO to stream this bit in/out?  And of course, an accompanying LTO
>> test to check for this problem you're encountering would be appropriate.
>>
>
> I also have a limited knowledge of LTO. This seem to be the most straightforward way of doing it (atleast for me).

See how other bits in `struct function' are streamed in/out in LTO, for 
example in output_struct_function_base()

   bp_pack_value (&bp, fn->calls_alloca, 1);
   bp_pack_value (&bp, fn->calls_setjmp, 1);
   bp_pack_value (&bp, fn->va_list_fpr_size, 8);
   bp_pack_value (&bp, fn->va_list_gpr_size, 8);

and the corresponding in input_struct_function_base():

   fn->calls_alloca = bp_unpack_value (&bp, 1);
   fn->calls_setjmp = bp_unpack_value (&bp, 1);
   fn->va_list_fpr_size = bp_unpack_value (&bp, 8);
   fn->va_list_gpr_size = bp_unpack_value (&bp, 8);

Also, you will need a testcase to make sure later changes to the 
compiler do not break LTO wrt Cilk features you have added.

>
>>> +       /* We need frame pointer for all Cilk Plus functions that uses
>>> +	  Cilk Keywords.  */
>>> +       || (flag_enable_cilkplus && cfun->is_cilk_function)
>>
>> "need a frame pointer"
>>
>> "that use"
>>
>> s/Keywords/keywords/
>>
>
> It should be keywords, because you need frame-pointer for "_Cilk_spawn and _Cilk_sync" and "_Cilk_for"

I meant that you should lowercase the "K".

>
>>> +  /* This variable will tell whether we are on a spawn helper or not */
>>> +  unsigned int is_cilk_helper_function : 1;
>>
>> Where is this used?
>>
>
> Well, it is not used now but later on when I add Tools support it will be. I will remove it for now.

Yes, please.

>>> --- gcc/ipa-inline-analysis.c
>>> +++ gcc/ipa-inline-analysis.c
>>> @@ -1433,6 +1433,9 @@ initialize_inline_failed (struct cgraph_edge *e)
>>>       e->inline_failed = CIF_REDEFINED_EXTERN_INLINE;
>>>     else if (e->call_stmt_cannot_inline_p)
>>>       e->inline_failed = CIF_MISMATCHED_ARGUMENTS;
>>> +  else if (flag_enable_cilkplus && cfun && cfun->calls_spawn)
>>> +    /* We can't inline if the function is spawing a function.  */
>>> +    e->inline_failed = CIF_BODY_NOT_AVAILABLE;
>>
>> Hmmm, if we don't have a cfun, perhaps we should be sticking this
>> calls_spawn bit in the cgraph node.
>>
>> Richard?  Anyone?
>>
>
> When I am first setting this, I don't think cgraph is available.

See Richard's comment with regards to struct function and its 
availability via the callee edge.  Also see his comment regarding the 
inappropriate error message.

>>> @@ -3496,7 +3510,7 @@ struct GTY(()) tree_function_decl {
>>>        ???  The bitfield needs to be able to hold all target function
>>>   	  codes as well.  */
>>>     ENUM_BITFIELD(built_in_function) function_code : 11;
>>> -  ENUM_BITFIELD(built_in_class) built_in_class : 2;
>>> +  ENUM_BITFIELD(built_in_class) built_in_class  ;
>>
>> What's this for?
>>
>
> Added a new enum field called BUILT_IN_CILK so we need 3 bits instead of 2, since there are 5 fields instead of 4.

Hmm, yeah.  I see you added another field here:

> diff --git a/gcc/tree.h b/gcc/tree.h
> index 0058a4b..952362f 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -262,6 +262,7 @@ enum built_in_class
>    NOT_BUILT_IN = 0,
>    BUILT_IN_FRONTEND,
>    BUILT_IN_MD,
> +  BUILT_IN_CILK,
>    BUILT_IN_NORMAL
>  };

If you look at the comment above enum built_in_class, you will see that 
these classes specify which part of the compiler created the built-in 
(the frontend, the backend (MD), or a normal builtin).  I don't see how 
Cilk should be treated specially.  And even so, I don't see how you use 
this BUILT_IN_CILK class anywhere.

>>> +  if (DECL_BUILT_IN_CLASS (exp) == BUILT_IN_NORMAL)
>>> +    return DECL_FUNCTION_CODE (exp) == BUILT_IN_MEMCPY;
>>> +  return lang_hooks.cilkplus.spawnable_constructor (exp);
>>> +  return false;
>>
>
> That would be necessary for C++, but it returns false for C. So, should I take out this hook for now? Would prefer to keep it in

You can keep the hook, but do put a comment specifying that it's a place 
holder for C++.  And also, please remove the second return.

>> Are these two hooks ever set to anything but hook_bool_tree_false?  If
>> so, why the need for them?
>>
>
> Used in C++ but not in C.

Leave them.  Similar comment please.

>>> +     struct __cilkrts_worker {
>>> +       __cilkrts_stack_frame *volatile *volatile tail;
>>> +       __cilkrts_stack_frame *volatile *volatile head;
>>> +       __cilkrts_stack_frame *volatile *volatile exc;
>>> +       __cilkrts_stack_frame *volatile *volatile protected_tail;
>>> +       __cilkrts_stack_frame *volatile *ltq_limit;
>>
>> What's this "*volatile *volatile" stuff?
>>
>
> That is how the field is described in Cilk Runtime source (please see: http://gcc.gnu.org/svn/gcc/branches/cilkplus/libcilkrts/include/internal/abi.h)

Ok, I'm not a language expert, I presume this is specifying volatile on 
both indirections and is proper form.

>>> +/* Gimplifies the cilk_sync expression passed in *EXPR_P.  Returns
>> GS_ALL_DONE
>>> +   when finished.  */
>>> +
>>> +int
>>> +gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p
>>> +		    ATTRIBUTE_UNUSED)
>>> +{
>>> +  tree sync_expr = build_cilk_sync ();
>>> +  *expr_p = NULL_TREE;
>>> +  gimplify_and_add (sync_expr, pre_p);
>>> +  return GS_ALL_DONE;
>>
>> I'm not a big fan of functions that always return the same thing.  The
>> caller should set GS_ALL_DONE accordingly.
>>
>
> The reason why I did this is that, there is a generic empty hook for this in langhooks.c that returns int. So, to reduce the number of code addition, I just made it return int. Also in future, if i want to add more things, having a return value I thought would be helpful.

Use void.  Set GS_ALL_DONE in the caller.  You can add the return value 
when you use it in the future.

Aldy