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

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

> -----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.

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.

> 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)
 
> 
> > +  /* 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.

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. 

> 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.

> For the memory barrier stuff, we already have Andrew's atomic and memory
> model infrastructure which I think should be enough to model whatever you are
> expanding into RTL here.  But I may be wrong...
> 
> Aldy