This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 08/21/2013 09:31 AM, Aldy Hernandez wrote:

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? :))
Agreed. While we don't have a strict policy for testing new features, adding tests for this kind of stuff is highly encouraged. Not everyone doing GCC development is going to be familiar enough with Cilk+ and how their patch might interact with the Cilk+ support.

Having tests in the testsuite makes it much less likely someone will break the Cilk+ support accidentally.

+      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.
Yea, that would definitely be a problem (walking every expression looking for cilk spawns inside). Having gimple nodes for these things would seem to make sense to me as well.


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.
Yea, we *really* want the gimplification & checking separated. If we think about where Andrew's proposals take us, then the checking needs to move into the front-end. gimplification should be relegated, to the fullest extent possible, to converting the IL down to gimple.




Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]