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




--- gcc/expr.c
+++ gcc/expr.c
@@ -9569,6 +9569,21 @@ expand_expr_real_1 (tree exp, rtx target, enum
machine_mode tmode,
  	}

        return expand_constructor (exp, target, modifier, false);
+    case INDIRECT_REF:
+      {
+	tree exp1 = TREE_OPERAND (exp, 0);
+	if (modifier != EXPAND_WRITE)
+	  {
+	    tree t = fold_read_from_constant_string (exp);
+	    if (t)
+	      return expand_expr (t, target, tmode, modifier);
+	  }
+	op0 = expand_expr (exp1, NULL_RTX, VOIDmode, EXPAND_SUM);
+	op0 = memory_address (mode, op0);
+	temp = gen_rtx_MEM (mode, op0);
+	set_mem_attributes (temp, exp, 0);
+	return temp;
+      }

Ughhh, what's the rationale for this?  Are generic changes to
expand_expr really needed?

Yes, I am expanding some variables of type "ptr->value" and those are emitted as INDIRECT_REF.

The fact that you are getting an INDIRECT_REF this late in the game is suspect.

Are you building with ENABLE_CHECKING, because it seems this should have been caught. See the places in tree-cfg.c with this:

    case INDIRECT_REF:
      error ("INDIRECT_REF in gimple IL");
      return t;


+  /* During LTO, the is_cilk_function flag gets cleared.
+     If __cilkrts_pop_frame is called, then this definitely must be a
+     cilk function.  */
+  if (cfun)
+    cfun->is_cilk_function = 1;

I don't know much about our LTO implementation, but perhaps you need to
teach LTO to stream this bit in/out?  And of course, an accompanying LTO
test to check for this problem you're encountering would be appropriate.


I also have a limited knowledge of LTO. This seem to be the most straightforward way of doing it (atleast for me).

See how other bits in `struct function' are streamed in/out in LTO, for example in output_struct_function_base()

  bp_pack_value (&bp, fn->calls_alloca, 1);
  bp_pack_value (&bp, fn->calls_setjmp, 1);
  bp_pack_value (&bp, fn->va_list_fpr_size, 8);
  bp_pack_value (&bp, fn->va_list_gpr_size, 8);

and the corresponding in input_struct_function_base():

  fn->calls_alloca = bp_unpack_value (&bp, 1);
  fn->calls_setjmp = bp_unpack_value (&bp, 1);
  fn->va_list_fpr_size = bp_unpack_value (&bp, 8);
  fn->va_list_gpr_size = bp_unpack_value (&bp, 8);

Also, you will need a testcase to make sure later changes to the compiler do not break LTO wrt Cilk features you have added.


+       /* We need frame pointer for all Cilk Plus functions that uses
+	  Cilk Keywords.  */
+       || (flag_enable_cilkplus && cfun->is_cilk_function)

"need a frame pointer"

"that use"

s/Keywords/keywords/


It should be keywords, because you need frame-pointer for "_Cilk_spawn and _Cilk_sync" and "_Cilk_for"

I meant that you should lowercase the "K".


+  /* This variable will tell whether we are on a spawn helper or not */
+  unsigned int is_cilk_helper_function : 1;

Where is this used?


Well, it is not used now but later on when I add Tools support it will be. I will remove it for now.

Yes, please.

--- gcc/ipa-inline-analysis.c
+++ 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_spawn)
+    /* We can't inline if the function is spawing a function.  */
+    e->inline_failed = CIF_BODY_NOT_AVAILABLE;

Hmmm, if we don't have a cfun, perhaps we should be sticking this
calls_spawn bit in the cgraph node.

Richard?  Anyone?


When I am first setting this, I don't think cgraph is available.

See Richard's comment with regards to struct function and its availability via the callee edge. Also see his comment regarding the inappropriate error message.

@@ -3496,7 +3510,7 @@ struct GTY(()) tree_function_decl {
       ???  The bitfield needs to be able to hold all target function
  	  codes as well.  */
    ENUM_BITFIELD(built_in_function) function_code : 11;
-  ENUM_BITFIELD(built_in_class) built_in_class : 2;
+  ENUM_BITFIELD(built_in_class) built_in_class  ;

What's this for?


Added a new enum field called BUILT_IN_CILK so we need 3 bits instead of 2, since there are 5 fields instead of 4.

Hmm, yeah.  I see you added another field here:

diff --git a/gcc/tree.h b/gcc/tree.h
index 0058a4b..952362f 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -262,6 +262,7 @@ enum built_in_class
   NOT_BUILT_IN = 0,
   BUILT_IN_FRONTEND,
   BUILT_IN_MD,
+  BUILT_IN_CILK,
   BUILT_IN_NORMAL
 };

If you look at the comment above enum built_in_class, you will see that these classes specify which part of the compiler created the built-in (the frontend, the backend (MD), or a normal builtin). I don't see how Cilk should be treated specially. And even so, I don't see how you use this BUILT_IN_CILK class anywhere.

+  if (DECL_BUILT_IN_CLASS (exp) == BUILT_IN_NORMAL)
+    return DECL_FUNCTION_CODE (exp) == BUILT_IN_MEMCPY;
+  return lang_hooks.cilkplus.spawnable_constructor (exp);
+  return false;


That would be necessary for C++, but it returns false for C. So, should I take out this hook for now? Would prefer to keep it in

You can keep the hook, but do put a comment specifying that it's a place holder for C++. And also, please remove the second return.

Are these two hooks ever set to anything but hook_bool_tree_false?  If
so, why the need for them?


Used in C++ but not in C.

Leave them.  Similar comment please.

+     struct __cilkrts_worker {
+       __cilkrts_stack_frame *volatile *volatile tail;
+       __cilkrts_stack_frame *volatile *volatile head;
+       __cilkrts_stack_frame *volatile *volatile exc;
+       __cilkrts_stack_frame *volatile *volatile protected_tail;
+       __cilkrts_stack_frame *volatile *ltq_limit;

What's this "*volatile *volatile" stuff?


That is how the field is described in Cilk Runtime source (please see: http://gcc.gnu.org/svn/gcc/branches/cilkplus/libcilkrts/include/internal/abi.h)

Ok, I'm not a language expert, I presume this is specifying volatile on both indirections and is proper form.

+/* Gimplifies the cilk_sync expression passed in *EXPR_P.  Returns
GS_ALL_DONE
+   when finished.  */
+
+int
+gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p
+		    ATTRIBUTE_UNUSED)
+{
+  tree sync_expr = build_cilk_sync ();
+  *expr_p = NULL_TREE;
+  gimplify_and_add (sync_expr, pre_p);
+  return GS_ALL_DONE;

I'm not a big fan of functions that always return the same thing.  The
caller should set GS_ALL_DONE accordingly.


The reason why I did this is that, there is a generic empty hook for this in langhooks.c that returns int. So, to reduce the number of code addition, I just made it return int. Also in future, if i want to add more things, having a return value I thought would be helpful.

Use void. Set GS_ALL_DONE in the caller. You can add the return value when you use it in the future.

Aldy


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