Re: [PATCH] _Cilk_for for C and C++

[Jeff Law <law at redhat dot com>]

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