Patchwork PR rtl-optimization/51771: revert TM returns twice kludge

login
register
mail settings
Submitter Aldy Hernandez
Date Dec. 5, 2012, 9:06 p.m.
Message ID <50BFB764.6050303@gmail.com>
Download mbox | patch
Permalink /patch/203962/
State New
Headers show

Comments

Aldy Hernandez - Dec. 5, 2012, 9:06 p.m.
As mentioned in the PR, this seems to no longer be an issue after we 
redesigned the TM abnormal edges (in the uninstrumented code path patchset).

Tested with the STAMP benchmark for both 1 and 4 threads, as well as 
with the GCC regression suite.

OK for trunk?
commit 462e1c25a81bfb575a9113e4d117c5972c827715
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Mon Dec 3 13:03:16 2012 -0600

    	PR rtl-optimization/51771
    	Revert:
    	* builtin-attrs.def (ATTR_RETURNS_TWICE, ATTR_TM_NOTHROW_RT_LIST): New.
    	* gtm-builtins.def (BUILT_IN_TM_START): Add returns-twice attrib.
Richard Henderson - Dec. 7, 2012, 8:04 p.m.
On 2012-12-05 15:06, Aldy Hernandez wrote:
>     	PR rtl-optimization/51771
>     	Revert:
>     	* builtin-attrs.def (ATTR_RETURNS_TWICE, ATTR_TM_NOTHROW_RT_LIST): New.
>     	* gtm-builtins.def (BUILT_IN_TM_START): Add returns-twice attrib.

Are we absolutely sure that the rtl edges are fixed?
Fixing only the gimple level edges doesn't really help the regsister allocator.


r~
Aldy Hernandez - Dec. 7, 2012, 8:11 p.m.
On 12/07/12 14:04, Richard Henderson wrote:
> On 2012-12-05 15:06, Aldy Hernandez wrote:
>>      	PR rtl-optimization/51771
>>      	Revert:
>>      	* builtin-attrs.def (ATTR_RETURNS_TWICE, ATTR_TM_NOTHROW_RT_LIST): New.
>>      	* gtm-builtins.def (BUILT_IN_TM_START): Add returns-twice attrib.
> Are we absolutely sure that the rtl edges are fixed?
> Fixing only the gimple level edges doesn't really help the regsister allocator.

I'm actually sure that the rtl edges are NOT fixed in the rtl :).

All the back edges are removed prior to generating RTL in 
gimple_expand_cfg(), seemingly by design:

       /* At the moment not all abnormal edges match the RTL
          representation.  It is safe to remove them here as
          find_many_sub_basic_blocks will rediscover them.
          In the future we should get this fixed properly.  */

What I was assuming was that the returns-twice kludge was papering over 
some other issue, because we were never sure that the lack of edges was 
actually causing the problem in the PR.  I can't find any tests 
(internal or external) that need the returns-twice hack.

I am hoping that by removing this hack we can find actual problems.

However, if you believe the RTL edge dropping in gimple_expand_cfg() is 
problematic, I can look into fixing this, though it would be nice to 
have a test I can work off of.

Aldy
Richard Henderson - Dec. 7, 2012, 8:25 p.m.
On 2012-12-07 14:11, Aldy Hernandez wrote:
> I am hoping that by removing this hack we can find actual problems.
> 
> However, if you believe the RTL edge dropping in gimple_expand_cfg() is problematic, I can look into fixing this, though it would be nice to have a test I can work off of.

Lets leave this for 4.9 work then.


r~

Patch

diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def
index 545b6fd..d149e26 100644
--- a/gcc/builtin-attrs.def
+++ b/gcc/builtin-attrs.def
@@ -107,7 +107,6 @@  DEF_ATTR_IDENT (ATTR_STRFTIME, "strftime")
 DEF_ATTR_IDENT (ATTR_TYPEGENERIC, "type generic")
 DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm")
 DEF_ATTR_IDENT (ATTR_TM_TMPURE, "transaction_pure")
-DEF_ATTR_IDENT (ATTR_RETURNS_TWICE, "returns_twice")
 
 DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL)
 
@@ -253,8 +252,6 @@  DEF_ATTR_TREE_LIST (ATTR_TM_NORETURN_NOTHROW_LIST,
 		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_NORETURN_NOTHROW_LIST)
 DEF_ATTR_TREE_LIST (ATTR_TM_CONST_NOTHROW_LIST,
 		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_CONST_NOTHROW_LIST)
-DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_RT_LIST,
-		    ATTR_RETURNS_TWICE, ATTR_NULL, ATTR_TM_NOTHROW_LIST)
 
 /* Same attributes used for BUILT_IN_MALLOC except with TM_PURE thrown in.  */
 DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_NOTHROW_LIST,
diff --git a/gcc/gtm-builtins.def b/gcc/gtm-builtins.def
index 171019e..7dcad03 100644
--- a/gcc/gtm-builtins.def
+++ b/gcc/gtm-builtins.def
@@ -1,5 +1,5 @@ 
 DEF_TM_BUILTIN (BUILT_IN_TM_START, "_ITM_beginTransaction",
-		BT_FN_UINT32_UINT32_VAR, ATTR_TM_NOTHROW_RT_LIST)
+		BT_FN_UINT32_UINT32_VAR, ATTR_TM_NOTHROW_LIST)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_COMMIT, "_ITM_commitTransaction",
 		BT_FN_VOID, ATTR_TM_NOTHROW_LIST)