This is the mail archive of the 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_for for C and C++

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.


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