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] |
Attached, please find a fixed patch and ChangeLog entries. My answers to your questions are given below. Thanks, Balaji V. Iyer. > -----Original Message----- > From: Aldy Hernandez [mailto:aldyh@redhat.com] > Sent: Monday, August 19, 2013 6:19 PM > To: Iyer, Balaji V > Cc: rth@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C > > > @@ -960,6 +960,7 @@ SCEV_H = tree-scalar-evolution.h $(GGC_H) > > tree-chrec.h $(PARAMS_H) OMEGA_H = omega.h $(PARAMS_H) > > TREE_DATA_REF_H = tree-data-ref.h $(OMEGA_H) graphds.h $(SCEV_H) > > TREE_INLINE_H = tree-inline.h > > +CILK_H = cilk.h > > REAL_H = real.h $(MACHMODE_H) > > IRA_INT_H = ira.h ira-int.h $(CFGLOOP_H) alloc-pool.h > > Doesn't cilk.h depend on tree.h? So shouldn't that be: > > CILK_H = cilk.h $(TREE_H) > > Which would mean that whoever includes cilk.h doesn't need to include tree.h. > Although... what I would prefer is not to include tree.h from cilk.h at all, and > have the caller/includer of cilk.h include tree.h first. > Ok. Fixed as you requested > > +c-family/cilk.o : c-family/cilk.c $(TREE_H) $(SYSTEM_H) $(CONFIG_H) > toplev.h \ > > + $(TREE_H) coretypes.h tree-iterator.h $(TREE_INLINE_H) $(CGRAPH_H) > \ > > + $(DIAGNOSTIC_CORE_H) $(GIMPLE_H) $(CILK_H) $(C_COMMON_H) > langhooks.h > > TREE_H is duplicated. > Fixed. > Also, you are using DIAGNOSTIC_CORE_H whereas c-family/cilk.c is using > Fixed. > > +#include "diagnostic.h" > > > +/* Cilk Keywords builtins. */ > > +#include "cilk-builtins.def" > > keywords should be lowercase > Fixed. > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index > > dc430c3..ab77fa4 100644 > > --- a/gcc/c-family/c-common.h > > +++ b/gcc/c-family/c-common.h > > @@ -148,6 +148,9 @@ enum rid > > /* C++11 */ > > RID_CONSTEXPR, RID_DECLTYPE, RID_NOEXCEPT, RID_NULLPTR, > > RID_STATIC_ASSERT, > > > > + /* Cilk Plus Keywords. */ > > + RID_CILK_SPAWN, RID_CILK_SYNC, > > + > > Same here. Fixed. > > > +/* In cilk.c. */ > > +extern tree insert_cilk_frame (tree); extern void cilk_init_builtins > > +(void); extern int gimplify_cilk_spawn (tree *, gimple_seq *, > > +gimple_seq *); extern int gimplify_cilk_sync (tree *, gimple_seq *, > > +gimple_seq *); extern void c_cilk_install_body_w_frame_cleanup (tree, > > +tree); extern bool cilk_valid_spawn (tree *); extern bool > > +cilkplus_set_spawn_marker (location_t, tree); > > You should be consistent with the prefix throughout the entire patch. > For instance, now you have cilk_init_builtins() but cilkplus_set_spawn_marker(). > Do whatever you like, but be consistent. > > Also, insert_cilk_frame-- the prefix should be well...as a prefix should be...at the > beginning. However, gimplify_cilk_spawn/sync is ok since that is in keeping with > the rest of the gimplify.c style. > I have renamed cilkplus_set_spawn_marker to cilk_set_spawn_marker. Well, insert_cilk_frame seem to flow correctly than saying "cilk_insert_frame." Will it be OK if I have "cilk" somewhere. > > + /* IF the function has _Cilk_spawn in front of a function call inside it > > + i.e. it is a spawning function, then add the appropriate Cilk plus > > + functions inside. */ > > Lowercase the F in IF. > Fixed. > > + c_parser_skip_until_found (parser, CPP_SEMICOLON, "expected > %<;%>"); > > + if (!flag_enable_cilkplus) > > + error_at (loc, "-fcilkplus must be enabled to use _Cilk_sync"); > > The error message should use %<_Cilk_sync> > Ok. I assume you wanted to make it %<_Cilk_sync%>.... > > + case RID_CILK_SPAWN: > > + c_parser_consume_token (parser); > > + if (!flag_enable_cilkplus) > > + { > > + error_at (loc, "-fcilkplus must be enabled to use _Cilk_spawn"); > > + expr = c_parser_postfix_expression (parser); > > + expr.value = error_mark_node; > > Similarly with _Cilk_spawn. > ... did the same to _Cilk_spawn also. > > + if (c_parser_peek_token (parser)->keyword == RID_CILK_SPAWN) > > + { > > + error_at (input_location, "consecutive _Cilk_spawn keywords " > > + "are not permitted"); > > Similarly here-- for that matter, check the rest of the patch for any keywords in > error messages, they should have %<blah>. > > Also, avoid input_location when possible. Surely, you can infer the actual > location from the parser. > Yes, I have replaced them all, with the exception of gimplify.c. Please see my note below for why. > > + if (flag_enable_cilkplus && retval && TREE_CODE (retval) == > CILK_SPAWN_STMT) > > + { > > + error_at (loc, "use of _Cilk_spawn in return statement is not allowed"); > > + return error_mark_node; > > s/in return/in a return/. Also, use %<_Cilk_spawn>. > Fixed both. > > +/* Returns a tree of type CILK_SYNC_STMT if Cilk Plus is enabled. Otherwise > > + return error_mark_node. */ > > + > > +tree > > +c_build_cilk_sync (void) > > +{ > > + tree sync = build0 (CILK_SYNC_STMT, void_type_node); > > + TREE_SIDE_EFFECTS (sync) = 1; > > + return sync; > > +} > > Code seems correct and comment wrong. Adjust accordingly. > Fixed. > > diff --git a/gcc/cilk.h b/gcc/cilk.h > > new file mode 100644 > > index 0000000..038316a > > --- /dev/null > > +++ b/gcc/cilk.h > > @@ -0,0 +1,94 @@ > > +/* This file is part of the Intel(R) Cilk(TM) Plus support > > + This file contains Cilk Support files. > > + Copyright (C) 2013 Free Software Foundation, Inc. > > Only one space after 2013. Similarly in other files. > Fixed. > > + > > +#ifndef GCC_CILK_H > > +#define GCC_CILK_H > > + > > +#include "tree.h" > > As mentioned earlier, cilk.h should not include tree.h, the caller should. > Fixed. > > +/* Frame status bits known to compiler. */ > > +#define CILK_FRAME_STOLEN 0x01 > > Unused? > Yup, so removed. > > +this keyword with a set of functions that are stored in the Cilk Runtime. > > Lowercase "Runtime". > Fixed. > > +@file{c-family/cilk.c}. In the spawn-helper, the gimplification > > +function @code{gimplify_call_expr}, inserts a function call > @code{__cilkrts_detach}. > > +This function is expanded by @code{builtin_expand_cilk_detach} > > +located in @file{c-family/cilk.c}. > > + > > +@item @code{_Cilk_sync}: > > +@code{_Cilk_sync} is parsed like a keyword. During gimplification, > > +the function @code{gimplify_cilk_sync} in @file{c-family/cilk.c}, > > +will replace this keyword with a set of functions that are stored in the Cilk > Runtime. > > +One of the internal functions inserted during gimplification, > > +@code{__cilkrts_pop_frame} must be expanded by the compiler and is > > +done by @code{builtin_expand_cilk_pop_frame} in @file{cilk-common.c}. > > Here and elsewhere in the documentation... It's up to you, but I would prefer to > remove reference to the actual file the functions are defined in, because > functions could be rearranged later. > I remember a while back someone was telling me that I should put that. > > + case CILK_SYNC_STMT: > > + { > > + if (flag_enable_cilkplus) > > + { > > + if (!cfun->cilk_frame_decl) > > + { > > + error_at (input_location, "expected _Cilk_spawn before " > > + "_Cilk_sync"); > > + ret = GS_ERROR; > > + } > > + else > > + ret = (enum gimplify_status) > > + lang_hooks.cilkplus.gimplify_cilk_sync (expr_p, pre_p, > > + post_p); > > + break; > > + } > > + else > > + /* _Cilk_sync without Cilk Plus enabling should be caught by > > + the parser. */ > > + gcc_unreachable (); > > The check for !flag_enable_cilkplus is not necessary, as well as the > gcc_unreachable. You shouldn't be checking for flag_enable_cilkplus in the > gimplifier; an earlier pass (parser?) should've caught this. See how > TRANSACTION_EXPR is handled. > OK. I fixed it. > Also, I would use the location of the CILK_SYNC_STMT instead of > input_location. > In gimplify_expr function, the input_location is set to EXPR_LOCATION (*expr_p), which is the location of the CILK SYNC STMT. > > diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c > > index 9a36292..7c63cc8 100644 > > --- a/gcc/ipa-inline-analysis.c > > +++ b/gcc/ipa-inline-analysis.c > > @@ -1433,6 +1433,9 @@ initialize_inline_failed (struct cgraph_edge *e) > > e->inline_failed = CIF_REDEFINED_EXTERN_INLINE; > > else if (e->call_stmt_cannot_inline_p) > > e->inline_failed = CIF_MISMATCHED_ARGUMENTS; > > + else if (flag_enable_cilkplus && cfun && cfun->calls_cilk_spawn) > > + /* We can't inline if the function is spawing a function. */ > > + e->inline_failed = CIF_FUNCTION_NOT_INLINABLE; > > Didn't rth mention that instead of looking through cfun (which may or may not > be there), you could check the callee/caller edge? > Yes, If I understood correctly, he said I was putting the wrong message in why inline failed. So I replaced it with CIF_FUNCTION_NOT_INLINABLE. > > +void > > +lhd_install_body_with_frame_cleanup (tree, tree) { > > + return; > > +} > > As mentioned before, avoid empty returns. > Fixed. > > + /* Returns true if the call expr passed is a spawned function call. > > + */ > > Here and elsewhere, you probably want s/call expr/CALL_EXPR/g > Fixed. > > diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h index > > b65dee9..32bedac 100644 > > --- a/gcc/tree-inline.h > > +++ b/gcc/tree-inline.h > > @@ -123,6 +123,10 @@ typedef struct copy_body_data > > the originals have been mapped to a value rather than to a > > variable. */ > > struct pointer_map_t *debug_map; > > + > > + /* Cilk keywords currently need to replace some variables that > > + ordinary nested functions do not. */ bool remap_var_for_cilk; > > } copy_body_data; > > > > Where is this field used? > IN function cilk_outline in c-family/cilk.c > > diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index > > 7745f73..f449c68 100644 > > --- a/gcc/tree-pretty-print.c > > +++ b/gcc/tree-pretty-print.c > > @@ -2403,6 +2403,14 @@ dump_generic_node (pretty_printer *buffer, tree > node, int spc, int flags, > > dump_block_node (buffer, node, spc, flags); > > break; > > > > + case CILK_SPAWN_STMT: > > + pp_string (buffer, "_Cilk_spawn "); > > + dump_generic_node (buffer, TREE_OPERAND (node, 0), spc, flags, false); > > + break; > > + > > + case CILK_SYNC_STMT: > > + pp_string (buffer, "_Cilk_sync;"); > > + break; > > default: > > NIY; > > > In keeping with the present code in this switch statement, empty line before the > default. > Fixed. > > +/* Cilk spawn expression > > + Operand 0 is the CALL_EXPR. */ > > +DEFTREECODE (CILK_SPAWN_STMT, "cilk_spawn_stmt", tcc_statement, 1) > > + > > +/* Cilk Sync Statement: Does not have any operands. */ DEFTREECODE > > +(CILK_SYNC_STMT, "cilk_sync_stmt", tcc_expression, 0) > > Be consistent. "Cilk spawn" but then "Cilk Sync"? Also "expression" in one, but > "statement" in the next? Also, Statement should be lowercase, as well as Sync. > Fixed. > > +/* Cilk Keywords accessors. */ > > +#define CILK_SPAWN_FN(NODE) TREE_OPERAND > (CILK_SPAWN_STMT_CHECK > > +(NODE), 0) > > + > > Lowercase "Keywords". > Fixed. > > + if (!current_function_decl) > > + { > > extra whitespace after { > Fixed. > > + else if (TREE_CODE (fcall) != CALL_EXPR) > > + { > > extra whitespace after { > Fixed. > > diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c > > b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c > > new file mode 100644 > > index 0000000..daf932e > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c > > @@ -0,0 +1,81 @@ > > +/* { dg-do compile } */ > > +/* { dg-do run { target { i?86-*-* x86_64-*-* arm*-*-* } } } */ > > +/* { dg-options "-fcilkplus -w" } */ > > +/* { dg-options "-lcilkrts" { target { i?86-*-* x86_64-*-* arm*-*-* } > > +} } */ > > + > > +#include <stdio.h> > > +#include <stdlib.h> > > +#define DEFAULT_VALUE "30" > > + > > +int fib (char *n_char) > > +{ > > + int n; > > + char n_char_minus_one[20], n_char_minus_two[20]; > > + if (n_char) > > + n = atoi (n_char); > > + else > > + n = atoi(DEFAULT_VALUE); > > + > > + if (n < 2) > > + return n; > > + else > > + { > > Extra whitespace after last { > > > diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c > > b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c > > new file mode 100644 > > index 0000000..465d1da > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c > > @@ -0,0 +1,38 @@ > > +/* { dg-do compile } */ > > +/* { dg-do run { target { i?86-*-* x86_64-*-* arm*-*-* } } } */ > > +/* { dg-options "-fcilkplus" } */ > > +/* { dg-options "-lcilkrts" { target { i?86-*-* x86_64-*-* arm*-*-* } > > +} } */ > > + > > +void f0(volatile int *steal_flag) > > +{ > > same > > > +/* This function does whatever is necessary to make the compiler emit > > +a newly > > Extra space at end > > > +/* Return true if this is a tree which is allowed to contain a spawn as > > + operand 0. > > + A spawn call may be wrapped in a series of unary operations such > > + as conversions. These conversions need not be "useless" > > + to be disregarded because they are retained in the spawned > > + statement. They are bypassed only to look for a spawn > > + within. > > + A comparison to constant is simple enough to allow, and > > + is used to convert to bool. */ > > Please add empty lines between paragraphs. > > > +static bool > > +cilk_spawnable_constructor (tree exp) { > > + if (TREE_CODE (exp) != ADDR_EXPR) > > + return false; > > Is this function ever called with a non ADDR_EXPR? > Fixed. > > +/* Helper function for walk_tree. If *TP is a CILK_SPAWN_STMT, then > unwrap > > + this "wrapper" and *WALK_SUBTREES is set to 0. The function > > +returns > > two many spaces after "and" > > Also, do not document what you are doing in the code by repeating it. > For instance, instead of "*WALK_SUBTREES is set to 0", either omit this, or say > what is being accomplished by setting WALK_SUBTREES to 0. I would just omit > it. > Fixed. > > + /* Remove the CALL_EXPR from CILK_SPAWN_STMT wrapper and return > > + true. */ > > Again, don't document the obvious. No need to say "and return true". > That much is obvious from the code. > > + /* Happens with C++ TARGET_EXPR. */ if (exp == NULL_TREE) > > + return false; > > Extra space after false; > Fixed. > > + warn = !cilk_spawnable_constructor (CALL_EXPR_FN (exp)); > > + > > + /* The function address of a call may not be computed via a spawn. > > + Look at the arglist only, and only the second argument which > > + is the RHS of any plausible assignment or copy. The first > > + argument is the LHS. A third argument could be a size for > > + memcpy. This path supports op= in addition to =, only because > > + it is easy to do so. */ > > + if (call_expr_nargs (exp) < 2) > > + return false; > > + > > + exp = CALL_EXPR_ARG (exp, 0); > > + > > + STRIP_USELESS_TYPE_CONVERSION (exp); > > + > > + if (TREE_CODE (exp) == ADDR_EXPR) > > + exp = TREE_OPERAND (exp, 0); > > + > > + if (TREE_CODE (exp) == TARGET_EXPR) > > + exp = TARGET_EXPR_INITIAL (exp); > > + > > + if (!exp || !recognize_spawn (exp, exp0)) > > + return false; > > + > > + if (warn) > > + warning (0, "suspicious use of _Cilk_spawn"); > > This warning seems very ambiguous. Do you think perhaps you could issue a > more meaningful error in cilk_spawnable_constructor()? > Fixed the warning > > +/* This function will return a FNDECL using information from *WD. */ > > + > > +static tree > > +build_cilk_helper_decl (struct wrapper_data *wd) > > I think a more meaningful comment is "This function will build and return a > FUNCTION_DECL using..." > Fixed. > > + case LABEL_DECL: > > + error_at (EXPR_LOCATION (decl), "invalid use of label %q+D in spawn", > > + decl); > > + return error_mark_node; > > Be consistent. If you're using _Cilk_spawn then use that. And you probably > want %<_Cilk_spawn> or whatever. > Fixed. > > + /* Copy FROM the function containing the spawn... */ id.src_fn = > > + outer_fn; > > + > > + /* ...TO the wrapper. */ > > Lower case FROM and TO. > > In addition to this, I did the following from the other emails that I received: 1. Replaced all the build_* with create_* for the static functions used by the gimplify_cilk_sync and gimplify_cilk_spawn functions. 2. Removed semicolon from the pretty-print file for _Cilk_sync 3. Replaced the unwrap_cilk_sync_stmt with unwrap_cilk_spawn_stmt 4. Moved c_build_spawn_stmt and c_build_sync_stmt to cilk.c and dropped the c_ prefix. 5. Made cilk_call_setjmp a static function. 6. Make cilk_set_marker.. function inline. 7. I renamed cilk_valid_spawn to cilk_detect_spawn as I mentioned in the previous email. The recognize spawn is to check for spawn in C++ specific trees (e.g. AGGR_INIT_EXPR), and cilk_detect_spawn is to detect spawn in an expression. One is sort of the subset of the other. The only other thing I can think of is to have one function called "cilk_detect_spawn" and then redo this function for C++. But ~90% of the code will be replicated, which I think you (or someone in gcc mailing list) mentioned last time was not a good idea. 8. Did not mark (more like undid my previous marking of) CILK_SPAWN_STMT and CILK_SYNC_STMT as typed. So, is this OK for trunk? Tested on x86_64 and works fine.
Attachment:
ChangeLog.cilkplus
Description: ChangeLog.cilkplus
Attachment:
diff.txt
Description: diff.txt
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |