On 08/21/13 14:59, Iyer, Balaji V wrote:
-----Original Message----- From: Aldy Hernandez
[mailto:al...@redhat.com] Sent: Wednesday, August 21, 2013 11:31 AM
To: Iyer, Balaji V Cc: r...@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
