Re: [trans-mem] verify_ssa failed with noreturn attribute function

Re: [trans-mem] verify_ssa failed with noreturn attribute function

[Aldy Hernandez]

I forgot to CC the list.

Aldy

> In the testcase below we have a transaction that never returns.  The
> function ipa_tm_insert_gettmclone_call() will transform the call to
> funcNoReturn() into:
> 
> 	 __transaction [[relaxed]]
> 	<bb 3>:
> 	  D.2887_2 = __builtin__ITM_getTMCloneOrIrrevocable (funcNoReturn);
> 	  D.2888_3 = (void (*<T130>) (void)) D.2887_2;
> -->	  # VUSE <.MEM_1(D)>
> 	  D.2888_3 (); [in atomic]
> 
> When we call update_stmt() on the function call, the VUSE gets
> transformed into a VDEF, which eventually causes the SSA to be marked
> for renaming:
> 
> 	  D.2887_2 = __builtin__ITM_getTMCloneOrIrrevocable (funcNoReturn);
> 	  D.2888_3 = (void (*<T130>) (void)) D.2887_2;
> -->	  # .MEM = VDEF <.MEM_1(D)>
> 	  D.2888_3 (); [in atomic]
> 
> So we need to update SSA, but ipa_tm_insert_gettmclone_call() returns
> TRUE only if the function call was TM safe.  Eventually we ICE because
> we try to verify the SSA web, but it needs updating.
> 
> I don't understand why we only update the SSA on safe function calls in
> ipa_tm_insert_gettmclone_call(), especially since
> ipa_tm_insert_gettmclone_call() calls update_stmt() at the end, which
> may possibly dirty the SSA web.  IMO, we should always return true,
> which is the approach I take below.
> 
> Another option is returning need_ssa_update_p(), or better yet, just
> call update_ssa() only if need_ssa_update_p() returns true and get rid
> of keeping track of need_ssa_rename altogether.
> 
> Is this OK, or am I overlooking something?
> 
> 	* trans-mem.c (ipa_tm_insert_gettmclone_call): Always return true.
> 
> Index: testsuite/gcc.dg/tm/20100609.c
> ===================================================================
> --- testsuite/gcc.dg/tm/20100609.c	(revision 0)
> +++ testsuite/gcc.dg/tm/20100609.c	(revision 0)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fgnu-tm -O" } */
> +
> +extern void funcNoReturn() __attribute__ ((__noreturn__));
> +
> +int later;
> +
> +void MyFunc()
> +{
> +  __transaction [[relaxed]] {
> +        funcNoReturn();
> +        later=8;
> +  }
> +}
> Index: trans-mem.c
> ===================================================================
> --- trans-mem.c	(revision 160392)
> +++ trans-mem.c	(working copy)
> @@ -4096,7 +4096,7 @@ ipa_tm_insert_gettmclone_call (struct cg
>    gimple_call_set_fn (stmt, callfn);
>    update_stmt (stmt);
>  
> -  return safe;
> +  return true;
>  }
>  
>  /* Helper function for ipa_tm_transform_calls.  For a given BB,

Re: [trans-mem] verify_ssa failed with noreturn attribute function

[Richard Henderson]

On 06/10/2010 04:12 AM, Aldy Hernandez wrote:
>> I don't understand why we only update the SSA on safe function calls in
>> ipa_tm_insert_gettmclone_call(), especially since
>> ipa_tm_insert_gettmclone_call() calls update_stmt() at the end, which
>> may possibly dirty the SSA web.  IMO, we should always return true,
>> which is the approach I take below.

I don't recall the logic behind only updating if safe.  It does 
look wrong now though.

>> Another option is returning need_ssa_update_p(), or better yet, just
>> call update_ssa() only if need_ssa_update_p() returns true and get rid
>> of keeping track of need_ssa_rename altogether.

Those do look like attractive options.

>> 	* trans-mem.c (ipa_tm_insert_gettmclone_call): Always return true.

Ok.


r~