Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Jeff Law]

On 04/28/2017 08:31 AM, Xi Ruoyao wrote:

On 2017-04-25 09:30 -0600, Jeff Law wrote:


Given Richi's general agreement around the in_lto_p, let's go with the
patch on the trunk only.


Should I prepare (re-diff) a patch for current trunk?

If you want for the trunk, yes.




If you get positive feedback from Jan, then this can be backported to
gcc-7 after it's been on the trunk for at least a week.



I just noticed GCC 7.0 has been frozen.
Right.  It'd be for the maintenance release that will likely happen in 
the summer.

jeff

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Xi Ruoyao]

On 2017-04-25 09:30 -0600, Jeff Law wrote:

> Given Richi's general agreement around the in_lto_p, let's go with the 
> patch on the trunk only.

Should I prepare (re-diff) a patch for current trunk?

> If you get positive feedback from Jan, then this can be backported to 
> gcc-7 after it's been on the trunk for at least a week.
> 

I just noticed GCC 7.0 has been frozen.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Jeff Law]

On 04/14/2017 06:44 PM, Xi Ruoyao wrote:

On 2017-04-14 15:00 +0800, Xi Ruoyao wrote:

On 2017-04-13 09:05 +0200, Richard Biener wrote:


Did you verify LTO bootstrap still works with the patch?


I've just done a LTO bootstrapp (boarding a train :) ).
It works with my patch.


I've done dejagnu tests in lto.exp and built a Linux kernel
with lto bootstrapped GCC.   They seem good.
Given Richi's general agreement around the in_lto_p, let's go with the 
patch on the trunk only.


If you get positive feedback from Jan, then this can be backported to 
gcc-7 after it's been on the trunk for at least a week.


jeff

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Xi Ruoyao]

On 2017-04-14 15:00 +0800, Xi Ruoyao wrote:
> On 2017-04-13 09:05 +0200, Richard Biener wrote:
> 
> > Did you verify LTO bootstrap still works with the patch?
> 
> I've just done a LTO bootstrapp (boarding a train :) ). 
> It works with my patch.

I've done dejagnu tests in lto.exp and built a Linux kernel
with lto bootstrapped GCC.   They seem good.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Xi Ruoyao]

On 2017-04-13 09:05 +0200, Richard Biener wrote:

> Did you verify LTO bootstrap still works with the patch?

I've just done a LTO bootstrapp (boarding a train :) ). 
It works with my patch.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Richard Biener]

On Wed, Apr 12, 2017 at 11:16 PM, Jeff Law  wrote:
> On 04/07/2017 08:02 AM, Xi Ruoyao wrote:
>>
>> On 2017-04-06 11:12 -0600, Jeff Law wrote:
>>
>>> With the likely deprecation in mind, I've only done a cursory review of
>>> the changes -- mostly to verify that they hit Cilk+ paths only.
>>
>>
>>> What's the purpose behind changing when we set the in_lto_p flag?
>>
>>
>> Without that change, GCC with my patch ICEed with _Cilk_spawn and
>> -flto -O3 -fcilkplus since __cilkrts_stack_frame.ctx's type (array of void
>> *)
>> was not TYPE_STRUCTURAL_EQUALITY_P in lto stage.
>> If this change is not proper, I'll work on modifying my patch to work
>> without touching in_lto_p.
>
> It's certainly be preferable to not change in_lto_p-- unless Richi wants to
> chime in on the safety of setting in_lto_p earlier.
>
> I'm not familiar enough with the LTO interactions to know if movement of
> in_lto_p is safe.

It should be safe and even technically more correct given we now have
various conditionals in the type building routines in tree.c that check
in_lto_p and avoid setting TYPE_CANONICAL from them -- TYPE_CANONICAL
is re-computed later, and for the builtin types we first zero TYPE_CANONICAL
(see lto.c:read_cgraph_and_symbols).

Now... I think those checks are somewhat wrong given that the middle-end
_does_ create types later, hopefully not ones we use for alias purposes,
but I'm not 100% sure.  So it somewhat feels like a hack ;)

So in theory the change is a good one.  I'm still nervous at this stage.

Did you verify LTO bootstrap still works with the patch?

CCing Honza who fiddled with this last (and introduced all those
in_lto_p checks).

Thanks,
Richard.

> Jeff

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Jeff Law]

On 04/07/2017 08:02 AM, Xi Ruoyao wrote:

On 2017-04-06 11:12 -0600, Jeff Law wrote:


With the likely deprecation in mind, I've only done a cursory review of
the changes -- mostly to verify that they hit Cilk+ paths only.



What's the purpose behind changing when we set the in_lto_p flag?


Without that change, GCC with my patch ICEed with _Cilk_spawn and
-flto -O3 -fcilkplus since __cilkrts_stack_frame.ctx's type (array of void *)
was not TYPE_STRUCTURAL_EQUALITY_P in lto stage.

If this change is not proper, I'll work on modifying my patch to work
without touching in_lto_p.
It's certainly be preferable to not change in_lto_p-- unless Richi wants 
to chime in on the safety of setting in_lto_p earlier.


I'm not familiar enough with the LTO interactions to know if movement of 
in_lto_p is safe.


Jeff

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Xi Ruoyao]

On 2017-04-06 11:12 -0600, Jeff Law wrote:

> With the likely deprecation in mind, I've only done a cursory review of 
> the changes -- mostly to verify that they hit Cilk+ paths only.

> What's the purpose behind changing when we set the in_lto_p flag?

Without that change, GCC with my patch ICEed with _Cilk_spawn and
-flto -O3 -fcilkplus since __cilkrts_stack_frame.ctx's type (array of void *)
was not TYPE_STRUCTURAL_EQUALITY_P in lto stage.

If this change is not proper, I'll work on modifying my patch to work
without touching in_lto_p.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Jeff Law]

On 03/31/2017 07:50 AM, Xi Ruoyao wrote:

Hi,

I''ve sent this patch once 
().
But I haven't got any response.  I'd like to resend it instead of pinging, and 
explain it more.
There's a couple things going on here.   Cilk+ does not currently have a 
maintainer, thus review of Cilk+ changes can easily fall through the 
cracks.  Additionally, the plan is deprecate Cilk+ in gcc-7, so it's 
very very low on our priority list.


With the likely deprecation in mind, I've only done a cursory review of 
the changes -- mostly to verify that they hit Cilk+ paths only.


What's the purpose behind changing when we set the in_lto_p flag?

Jeff

[PATCH] Destroy arguments for _Cilk_spawn calling in the child (PR 80038)

[Xi Ruoyao]

Hi,

I''ve sent this patch once 
().
But I haven't got any response.  I'd like to resend it instead of pinging, and 
explain it more.

There was an ancient bug PR middle-end/60586 before r227423.  At that time GCC 
evaluated
parameters inside spawn worker, and violated Intel cilk spec.  To fix it, 
Balaji V. Iyer explictly
gimplified the "_Cilk_spawn" arguments in the caller, outside of "_cilk_spn_x" 
helpers.

However there is an unwanted side-effect with r227423.  Gimplifying arguments 
in the caller
produced destruction sequence of the arguments in the caller too.  So the 
caller would
destruct arguments even if the spawned callee is still running.  Then the 
program just dumps
core, or produces wrong result.

For example, NumericMonoid  produce 
wrong
answer in GCC 6/7.  And passing STL containers as _Cilk_spawn arguments would 
likely make
the program dump core.

This is also an Intel cilk spec violation, since the spec explicitly contrains 
the destruction
of arguments in the child.  See the spec

3.6 [4]:

> Any unnamed temporary variables created prior to the spawn point are not 
> destroyed until
> after the spawn point (i.e., *their destructors are invoked in the child*).

To fix this, I reverted r227423 at first.  But the revertion reintroduced PR 
60586.  So I re-fixed
it without violating the spec.  The pedigree operations are merged into 
built-in function
_cilkrts_detach like the version in cilk_fake.h within libcilkrts.  Then we 
just insert a call to it
in the exact correct location in GIMPLE instead of tree.  In the tree the 
argument evaluation
and function call is just a CALL_EXPR and we can not seperate them.  But in 
GIMPLE we can
insert detach sequence between them.

The patch has 500+ lines so I would attach it.

Although 80038 is a "c++" PR in the bugzilla, I think it should be "middle-end".
Maybe we should reclassify PR 80038 and modify the ChangeLog as well.

This patch has been bootstrapped and regtested in x86_64-linux-gnu.  Is it
OK for trunk?
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University
From dfbce2c4aafd4a555ef751af2d7422d2ba79189b Mon Sep 17 00:00:00 2001
From: Xi Ruoyao 
Date: Fri, 24 Mar 2017 04:35:23 +0800
Subject: [PATCH] Destroy temps for _Cilk_spawn calling in the child (PR
 c++/80038)

Revert r227423, and re-fix PR60586 without breaking cilk specs.

2017-03-24  Xi Ruoyao  

	PR c++/80038
	* c-family/c-common.h (cilk_gimplify_call_params_in_spawned_fn,
	  cilk_install_body_pedigree_operations): Remove prototypes.
	* c-family/c-gimplify.c (c_gimplify_expr): Remove the calls to
	  the function cilk_gimplify_call_params_in_spawned_fn.
	* c-family/cilk.c: (cilk_set_spawn_marker): Mark the function
	  calls which should be detached.
	  (cilk_gimplify_call_params_in_spawned_fn,
	   cilk_install_body_pedigree_operations): Remove function.
	  (gimplify_cilk_spawn): Add EXPR_STMT and CLEANUP_POINT_EXPR
	  unwrapping.
	* c/c-typeck.c (cilk_install_body_with_frame_cleanup):
	  Don't add pedigree operation and detach call here.
	* cp/cp-cilkplus.c (cilk_install_body_with_frame_cleanup): Ditto.
	* cp/cp-gimplify.c (cilk_cp_gimplify_call_params_in_spawned_fn):
	  Remove function.
	  (cp_gimplify_expr): Remove the calls to the function
	  cilk_cp_gimplify_call_params_in_spawned_fn.
	* cp/semantics.c: Preserve the flag of function calls should
	  be detached.
	* cilk_common.c (expand_builtin_cilk_detach): Move pedigree
	  operations here.
	* gimplify.c (gimplify_cilk_detach): New static function.
	  (gimplify_call_expr, gimplify_modify_expr): Call it for the
	  function calls should be detached.
	* lto/lto-lang.c (lto_init): Set in_lto_p earlier.
	* tree-core.h: Document new macro EXPR_CILK_SPAWN.
	* tree.h: Add new macro EXPR_CILK_SPAWN.
	* testsuite/g++.dg/cilk-plus/CK/pr80038.cc: New test.
---
 gcc/c-family/c-common.h  |   2 -
 gcc/c-family/c-gimplify.c|  10 +--
 gcc/c-family/cilk.c  | 102 +++
 gcc/c/c-typeck.c |   8 +--
 gcc/cilk-common.c|  49 +
 gcc/cp/cp-cilkplus.c |   6 +-
 gcc/cp/cp-gimplify.c |  40 ++-
 gcc/cp/semantics.c   |   2 +
 gcc/gimplify.c   |  21 ++
 gcc/lto/lto-lang.c   |   6 +-
 gcc/testsuite/g++.dg/cilk-plus/CK/pr80038.cc |  47 
 gcc/tree-core.h  |   4 ++
 gcc/tree.h   |   6 ++
 13 files changed, 150 insertions(+), 153 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cilk-plus/CK/pr80038.cc

diff --git