This is the mail archive of the gcc-patches@gcc.gnu.org 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 Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)


On 09/11/13 12:18, Iyer, Balaji V wrote:
Hello Everyone, Couple weeks back, I had submitted a patch for review
that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
the C compiler. I recently finished C++ implementation also. In this
email, I am attaching 2 patches: 1 for C (and the common parts for C
and C++) and 1 for C++. The C++ Changelog is labelled
cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
There isn't much changes in the C patch. Only noticeable changes
would be moving functions to the common parts so that C++ can use
them.

It passes all the tests and does not affect  (by affect I mean fail a
passing test or pass a failing one) any of the other tests in the
testsuite directory.
More random comments, questions & things to fix:

In cilk_common.c we have:
+/* Returns the value in structure FRAME pointed by the FIELD_NUMBER
+   (e.g. X.y).
+   FIELD_NUMBER is an index to the structure FRAME_PTR.  For details
+   about these fields, refer to cilk_trees structure in cilk.h and
+ cilk_init_builtins function in this file. Returns a TREE that is the type
+   of the field represented by FIELD_NUMBER.  */
+
+tree
+cilk_dot (tree frame, int field_number, bool volatil)
+{
+  tree field = cilk_trees[field_number];
+  field = fold_build3 (COMPONENT_REF, TREE_TYPE (field), frame, field,
+                      NULL_TREE);
+  TREE_THIS_VOLATILE (field) = volatil;
+  return field;
+}

There's no description of what the VOLATIL argument does. Similarly for cilk_arrow. From reading other code it appears to have the usual meaning of volatile, but it's best to be explicit.

In add_field:


+/* This function will add FIELD of type TYPE to a defined built-in
+   structure.  */
+
+static tree
+add_field (const char *name, tree type, tree fields)
+{
+  tree  t = get_identifier (name);
+  tree field = build_decl (BUILTINS_LOCATION, FIELD_DECL, t, type);
+  TREE_CHAIN (field) = fields;
+  return field;
+}
NAME is not documented. Nit: Just one space between the type specifier and the variable name on this line:

tree  t = get_identifier (name);


Presumably add_field is called well in advance of layout_decl :-)

install_builtin doesn't document CODE or PUBLISH in its comment header.


Presumably users never concern themselves with the cilkrts_pedigree structure, it's entirely hidden, right? So there's no way for a user to ask for an array of these things, right?


I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive. Why not compute the natural alignment for that structure and use that? Similarly for the cilkrts_stack_frame, except that it seems to use PREFERRED_STACK_BOUNDARY. Is there some reason why each shouldn't be aligned on whatever boundary the target would normally align those structures?

For the "ctx" array, presumably it's some kind of context. Where does the size of that array come from?

More generally is there any way we can ensure those structures are consistent between the runtime and the compiler? How often have those structures changed over time and how did y'all deal with the changes?


Presumably get_frame_arg is only used on calls into the cilk runtime, right and are all generated by GCC, and should always have the right number of arguments, should always be compatible, etc?!? If so, then those sanity checks should probably be asserts. Else it seems fine.


Comment for cilk_detach indicates it returns const0_rtx, but signature has "void" for the return type.

I'm assuming the code in expand_cilk_sync actually implements the pseudocode. I didn't verify every hunk of tree you built.

I'd been wondering about concurrent access to various structures in here. x86 has a fairly easy-to-use memory model. Has any thought been given to what would need to change to support the weaker memory models such as are found on PPC?

More tomorrow I hope.

jeff


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