On 11/18/13 14:50, Iyer, Balaji V wrote:
Attached, please find a refreshed patches (one for C and 1 for C++). The trunk
was "diffed" after Aldy's check in of pragma simd was in. So, now this patch is
only dependent on _Cilk_spawn and _Cilk_sync (mostly for execution of tests). They are
tested on x86_64 and works successfully.
Here are the fixed Changelog entries (C related changelogs are given first then
C++):
I'm just getting started on the C stuff. This likely will be a partial
review.
First, it looks like you've got a ton of unrelated cleanup work going on
in c-family/cilk.c. Function/parameter renaming and the like. If this
stuff is important, can it go forward independently? It's certainly
hard to find Cilk_for stuff in there with all the unrelated changes
going on. Some of the stuff seems to have moved into cilk.h which is
probably good too, but probably should be separated from the Cilk_for
support patch as well.
The semantics of cilk_check_loop_difference_type seem a bit odd. Can
you explain a bit more how those semantics were choosen? And given the
implementation, isn't it impossible for it to return NULL_TREE, meaning
that check in c_extract_cilk_for_fields is pointless?
Isn't cilk_simplify_tree just used in the validation code? Can it be
moved into that file and privatized? It probably needs a better name
too. I have to also admit, it looks pretty bogus to just start
stripping things like that. Oh yea, declaring and using tree_ssa_... is
not OK there, thus it's not ok to use STRIP_USELESS_TYPE_CONVERSION.
In general, could you do a pass over those functions with external
linkage and if they're only used in one file, move them to that file and
privatize them?
So Cilk_for has a type? That's the impression I get by looking at
cilk_loop_convert. In reality, I think it's just poorly named. I've
got to believe there's something, somewhere that will do equivalent
conversions for you :-)
There's a mismatch between direction tracking between
cilk_calc_forward_div_op and cilk_compute_incr_direction. THe former
handles -1, the latter never returns it. I'd probably prefer to see the
direction information represented as an ENUM anyway.
.
Presumably you're using nested functions to encapsulate the body for the
Cilk_for loop? Does the nested function have access to the parent's
context?
If the verification routines have to stay, then isn't there a lot of
commonality between (for example) cilk_set_incr_info and
c_check_cilk_loop_incr? Anything worth refactoring there? Probably not
I guess
I must have missed something -- when can we get a CONTINUE_STMT outside
a loop body? See cilk_resolve_continue_stmts.
In c-common.h, you added several prototypes for things in c-cilkplus.c.
We're trying to avoid doing that, instead favoring putting them into
c-cilkplus.h and including that where needed.
Similarly in c-tree.h
Is there any significant sharable code between parsing a normal C for
statement a a Cilk_for that we could factor out? What about the
validation code? Can any of that be shared with OpenMP? What about the
gimplification?
Are the syntatical differences so significant that we can't share any of
that code?
Any particular reason why the runtime uses different names for the 32bit
and 64bit Clik_for support? ISTM you wouldn't ever be binding the 32bit
and 64bit runtimes into a single application, so you could have just
called it __cilkrts_cilk_for and be done with it. I guess it's too late
to change that now. Or does the 32/64 split refer to the # bits
necessary to hold the iteration counter?
vertical whitespace removed after the cilk_arrow function, causing it to
end up immediately adjacent to the comment for the next function. I
suspect this was unintentional.
You define CILK_FOR_STMT and new gimplification routines to handle it.
Is there any way you can funnel all this through the OpenMP 4 support?
It looks like you've got unrelated cleanups going on in c-family/cilk.c.
Function renaming and such. Is that strictly related to Cilk_for
support? Much of it looks independent of Cilk_for support. Seems to me
like that should break out and go forward independently.
I'm going to have to look at this some more, but I wanted to give you as
much feedback as I could before I disappear for the holidays.
jeff