Even more review stuff. Are you keeping track of all this Balaji? :)
+ 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.
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]".
+ /* 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.
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.
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
