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

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

For some reason gcc-patches kicked me out. Here is a re-send.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Iyer, Balaji V
> Sent: Tuesday, August 27, 2013 5:27 PM
> To: 'Aldy Hernandez'
> Cc: rth@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
> 
> 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).
> 
> 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 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 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.
> 
> Also, I had added a couple more tests to catch a couple cases.
> 
> Is this OK for trunk?
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > owner@gcc.gnu.org] On Behalf Of Aldy Hernandez
> > Sent: Thursday, August 22, 2013 12:52 PM
> > 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
> >
> > 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

Attachment: patch.txt
Description: patch.txt

Attachment: ChangeLog.cilkplus
Description: ChangeLog.cilkplus