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

Aldy Hernandez aldyh@redhat.com
Thu Aug 22 16:53:00 GMT 2013


On 08/21/13 14:59, Iyer, Balaji V wrote:
>
>
>> -----Original Message----- From: Aldy Hernandez
>> [mailto:aldyh@redhat.com] Sent: Wednesday, August 21, 2013 11:31 AM
>> To: Iyer, Balaji V Cc: rth@redhat.com; Jeff Law;
>> gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Cilk Keywords
>> (_Cilk_spawn and _Cilk_sync) for C
>>
>> Even more review stuff.  Are you keeping track of all this Balaji?
>> :)
>>
>
> Yes I am. Please keep an eye out for a fixed patch soon.
>
>>> +  if (warn) +    warning (0, "suspicious use of _Cilk_spawn");
>>
>> First, as I've mentioned, this error message is very ambiguous. You
>> should strive to provide better error messages.  See my previous
>> comment on this same line of code.
>>
>> However... for all the checking you do in cilk_valid_spawn, I
>> don't see a single corresponding test.
>>
>
> Well, the name of the function is misleading.  I will fix that. I
> think it should call it "detect_cilk_spawn" instead
>
> What the function does it NOT to find whether there are syntax or
> other issues in the spawned statement, but to check if spawn is used
> in appropriate location. Here are some cases were you can use spawn
> (I am sure I am missing something):
>
> X = _Cilk_spawn foo ();
>
> _Cilk_spawn foo ()
>
> operator=(x, _Cilk_spawn foo ())
>
> and these things can be kept in different kind of trees and so
> adding this in individual tree's case statement can be a lot of
> code-addition and is error prone.

It still sounds like some sort of type checking best done at the source
level.  You can analyze all this in the parser as suggested.  Checking 
if spawn is used in the appropriate location, as you mention, should not 
be done in the gimplifier.

And btw, the reason you have to check all these variants (whether in a a 
MODIFY_EXPR, INIT_EXPR, CALL_EXPR, operator, etc) is because you are 
dealing with trees.  If you handled this as a gimple tuple, things would 
have already been simplified enough for you to only have to worry about 
GIMPLE_CALL.  That being said, I understand it would be more work to 
redesign things to work at the gimple level, but it is something we want 
to do eventually.

>
> The warning you see is more like an "heads up." I can take out of if
> you like. If you notice, when I see an error, I don't bother
> gimplifying the spawned function (but just let the compiler go ahead
> as a regular function call) thereby not creating a new nested
> function etc.

No, I don't want you to take it out.  For that matter (as I've suggested 
earlier up-thread), I would like you to expand on this and provide more 
verbose errors/warnings for each of the different things you can catch, 
instead of the the generic "suspicious" warning.

>
>> May I stress again the importance of tests-- which are especially
>> critical for new language features.  You don't want cilk silently
>> breaking thus rendering all your hard work moot, do you? :))
>>
>> You particularly need tests for all quirks described in the Cilk
>> Plus language specification around here:
>>
>> "A program is considered ill formed if the _Cilk_spawn form of
>> this expression appears other than in one of the following
>> contexts: [blah blah blah]".
>>
>
> I have several of those already (e.g. using spawn outside a
> function, spawning something that is not a function, etc)

I just didn't see any test for the "suspicious" warning.

>
>>
>>> +  /* Strip off any conversion to void.  It does not affect
>>> whether spawn +     is supported here.  */ +  if (TREE_CODE
>>> (exp) == CONVERT_EXPR && VOID_TYPE_P (TREE_TYPE
>> (exp)))
>>> +    exp = TREE_OPERAND (exp, 0);
>>
>> Don't you need to strip off various levels here with a loop?
>> Also, could any of the following do the job? STRIP_NOPS,
>> STRIP_TYPE_NOPS, STRIP_USELESS_TYPE_CONVERSION.
>>
>>> @@ -7086,6 +7087,19 @@ gimplify_expr (tree *expr_p, gimple_seq
>>> *pre_p,
>> gimple_seq *post_p,
>>> else if (ret != GS_UNHANDLED) break;
>>>
>>> +      if (flag_enable_cilkplus &&
>>> lang_hooks.cilkplus.cilk_valid_spawn (expr_p)) +	{ +	  /* If
>>> there are errors, there is no point in expanding the +
>>> _Cilk_spawn.  Just gimplify like a normal call expr.  */ +	  if
>>> (!seen_error ()) +	    { +	      ret = (enum gimplify_status) +
>>> lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
>> post_p);
>>> +	      if (ret != GS_UNHANDLED) +		continue; +	    } +	} +
>>
>> Oh, hell no!  You do realize you are drilling down and walking
>> every single expression being passed to the gimplifier to find
>> your spawn? That's not cool.  You need to find some way to
>> annotate expressions or do this more efficiently.  It may help to
>> bootstrap with -fcilkplus and do performance analysis, to make sure
>> you're not making the compiler slower on the non cilkplus code
>> path.
>>
>> Could you not let the gimplifier do its thing and add a case for
>> CILK_SPAWN_STMT where you do the unwrapping and everything else?
>> I do realize that cilk_valid_spawn() is doing all sorts of type
>> checking, and validation, but the gimplifier is really not the
>> place to do this.  When possible, you should do type checking as
>> close to the source as possible, thus-- at the parser.  See how
>> c_finish_omp_for() is called from the FE to do type checking,
>> build the OMP_FOR tree node, *and* do the add_stmt().  Perhaps you
>> need corresponding a c_finish_cilk_{spawn,sync}.  Definitely worth
>> looking into.  But I can tell you now, drilling down into every
>> expression being gimplified is a no-go.
>>
>
> Well, I think the name of the function is what that is misleading
> here.
>
> I am not recursing through the entire tree to find the spawn keyword
> here. What I am trying to see is if "*expr_p" is an INIT_EXPR, or
> TARGET_EXPR or a CALL_EXPR etc.

You are still iterating through every expression passed to 
gimplify_expr.  I mean, this is some of the stuff you do in 
cilk_valid_spawn:

+  /* Strip off any conversion to void.  It does not affect whether spawn
+     is supported here.  */
+  if (TREE_CODE (exp) == CONVERT_EXPR && VOID_TYPE_P (TREE_TYPE (exp)))
+    exp = TREE_OPERAND (exp, 0);
+
+  if (TREE_CODE (exp) == MODIFY_EXPR || TREE_CODE (exp) == INIT_EXPR)
+    exp = TREE_OPERAND (exp, 1);
+
+  while (cilk_ignorable_spawn_rhs_op (exp))
+    exp = TREE_OPERAND (exp, 0);

Whether we agree to go the gimple route or not, you need to rearrange 
this code in gimplify_expr to have a case for CILK_SPAWN_STMT instead of 
doing this ad-hoc iterating you're doing.

>
> I do agree with you about one thing. I should first check to see if
> the function has a _Cilk_spawn before I go through and check for
> individual trees. That can be done easily by looking at
> cfun->cilk_frame_decl != NULL_TREE. That change I will make in the
> next patch.

I thought you were already doing that.  See the top of cilk_valid_spawn():

+  /* If the function contains no Cilk code, this isn't a spawn.  */
+  if (!cfun->cilk_frame_decl)
+    return false;


>
>> Also, do you realy need two hooks to recognize spawns:
>> recognize_spawn and cilk_valid_spawn?  And are C/C++ so different
>> that you need a hook with different versions of each?
>>
>>> +/* Returns a setjmp CALL_EXPR with FRAME->context as its
>>> parameter. +*/ + +tree +cilk_call_setjmp (tree frame)
>>
>> Is this used anywhere else but in this file?  If not, please
>> declare static.
>>
>>> +/* Expands the __cilkrts_pop_frame function call stored in EXP.
>>>  +   Returns const0_rtx.  */ + +void
>>> +expand_builtin_cilk_pop_frame (tree exp)
>> [snip]
>>> +/* Expands the cilk_detach function call stored in EXP. Returns
>>> +const0_rtx.  */ + +void +expand_builtin_cilk_detach (tree exp)
>>
>> Do these builtins really have to be expanded into rtl?  Can this
>> not be modeled with trees or gimple?  Expansion into rtl should be
>> used for truly architecture dependent stuff that cannot be modeled
>> with anything higher level.
>>
>
> Again, the reason why I do it there because it is easier for me. I
> don't think it is causing a huge performance hit both in the
> compiler or the executable.

I realize it is easier for you, but so is hard-coding things throughout 
the compiler.  We need to look for the long term maintainability of the 
compiler.  I will let the appropriate maintainers chime in, but for the 
record I would much prefer to expand into trees instead of rtl.

Aldy



More information about the Gcc-patches mailing list