[PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Fang, Changpeng]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

Hi,

Attached is the patch to fix bug 44503: "control flow in the middle of basic block" with -fprefetch-loop-arrays.

The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop 
construction problem. When the current function has non local labels, there are no natural loops in the 
function. 

The patch passed bootstrapping and gcc regression tests on amd-linux64 systems.

Is it ok for the trunk?

Thanks,

Changpeng

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Do-not-form-natural-loops-when-non-local-labels-exis.patch --]
[-- Type: text/x-patch; name="0003-Do-not-form-natural-loops-when-non-local-labels-exis.patch", Size: 1003 bytes --]

From 91dd3df467e121426131cf477a77eb07d6062438 Mon Sep 17 00:00:00 2001
From: Changpeng Fang <chfang@houghton.(none)>
Date: Mon, 14 Jun 2010 16:46:06 -0700
Subject: [PATCH 3/3] Do not form natural loops when non-local labels exist in the current function

	*cfgloop.c (flow_loops_find): When the current function has non local
	labels, there are no natural loops in the function.
---
 gcc/cfgloop.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/gcc/cfgloop.c b/gcc/cfgloop.c
index 858e75b..d2ba445 100644
--- a/gcc/cfgloop.c
+++ b/gcc/cfgloop.c
@@ -394,6 +394,18 @@ flow_loops_find (struct loops *loops)
       return 1;
     }
 
+ /* When the current function has non local labels, there are no
+     natural loops in the function.  */
+  if (cfun->has_nonlocal_label)
+    {
+      init_loops_structure (loops, 1);
+
+      FOR_EACH_BB (bb)
+	bb->loop_father = loops->tree_root;
+
+      return 1;
+    }
+
   dfs_order = NULL;
   rc_order = NULL;
 
-- 
1.6.3.3

Re: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Jan Hubicka]

> Hi,
> 
> Attached is the patch to fix bug 44503: "control flow in the middle of basic block" with -fprefetch-loop-arrays.
> 
> The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
> call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop 
> construction problem. When the current function has non local labels, there are no natural loops in the 
> function. 

I would rather say it is CFG problem - for known builtins, like
__bultiln_prefetch we certainly don't need to produce nonlocal goto edges.

In fact this might be perhaps bundled in my "leaf" attribute patch I proposed
some time ago.

Honza
> 
> The patch passed bootstrapping and gcc regression tests on amd-linux64 systems.
> 
> Is it ok for the trunk?
> 
> Thanks,
> 
> Changpeng
Content-Description: 0003-Do-not-form-natural-loops-when-non-local-labels-exis.patch
> From 91dd3df467e121426131cf477a77eb07d6062438 Mon Sep 17 00:00:00 2001
> From: Changpeng Fang <chfang@houghton.(none)>
> Date: Mon, 14 Jun 2010 16:46:06 -0700
> Subject: [PATCH 3/3] Do not form natural loops when non-local labels exist in the current function
> 
> 	*cfgloop.c (flow_loops_find): When the current function has non local
> 	labels, there are no natural loops in the function.
> ---
>  gcc/cfgloop.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/gcc/cfgloop.c b/gcc/cfgloop.c
> index 858e75b..d2ba445 100644
> --- a/gcc/cfgloop.c
> +++ b/gcc/cfgloop.c
> @@ -394,6 +394,18 @@ flow_loops_find (struct loops *loops)
>        return 1;
>      }
>  
> + /* When the current function has non local labels, there are no
> +     natural loops in the function.  */
> +  if (cfun->has_nonlocal_label)
> +    {
> +      init_loops_structure (loops, 1);
> +
> +      FOR_EACH_BB (bb)
> +	bb->loop_father = loops->tree_root;
> +
> +      return 1;
> +    }
> +
>    dfs_order = NULL;
>    rc_order = NULL;
>  
> -- 
> 1.6.3.3
> 

RE: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Fang, Changpeng]

Hi,
>
> >Attached is the patch to fix bug 44503: "control flow in the middle of basic block" with -fprefetch-loop-arrays.
>
> >The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
> >call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop
> >construction problem. When the current function has non local labels, there are no natural loops in the
> >function.

>I would rather say it is CFG problem - for known builtins, like
>__bultiln_prefetch we certainly don't need to produce nonlocal goto edges.

You are right in this case. But in general, a function call may indirectly goto
the non-local labels. I didn't see any logic in gcc that handle this for loop 
construction.

>In fact this might be perhaps bundled in my "leaf" attribute patch I proposed
>some time ago.
But the problem occurs in the current gcc source.

Thanks,

Changpeng





Honza
>
> The patch passed bootstrapping and gcc regression tests on amd-linux64 systems.
>
> Is it ok for the trunk?
>
> Thanks,
>
> Changpeng
Content-Description: 0003-Do-not-form-natural-loops-when-non-local-labels-exis.patch
> From 91dd3df467e121426131cf477a77eb07d6062438 Mon Sep 17 00:00:00 2001
> From: Changpeng Fang <chfang@houghton.(none)>
> Date: Mon, 14 Jun 2010 16:46:06 -0700
> Subject: [PATCH 3/3] Do not form natural loops when non-local labels exist in the current function
>
>       *cfgloop.c (flow_loops_find): When the current function has non local
>       labels, there are no natural loops in the function.
> ---
>  gcc/cfgloop.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/gcc/cfgloop.c b/gcc/cfgloop.c
> index 858e75b..d2ba445 100644
> --- a/gcc/cfgloop.c
> +++ b/gcc/cfgloop.c
> @@ -394,6 +394,18 @@ flow_loops_find (struct loops *loops)
>        return 1;
>      }
>
> + /* When the current function has non local labels, there are no
> +     natural loops in the function.  */
> +  if (cfun->has_nonlocal_label)
> +    {
> +      init_loops_structure (loops, 1);
> +
> +      FOR_EACH_BB (bb)
> +     bb->loop_father = loops->tree_root;
> +
> +      return 1;
> +    }
> +
>    dfs_order = NULL;
>    rc_order = NULL;
>
> --
> 1.6.3.3
>

Re: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Zdenek Dvorak]

Hi,

> Attached is the patch to fix bug 44503: "control flow in the middle of basic block" with -fprefetch-loop-arrays.
> 
> The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
> call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop 
> construction problem. When the current function has non local labels, there are no natural loops in the 
> function. 

this does not make much sense to me; why should there be no natural loops?  Edges in CFG (conservatively)
correctly describe the control flow, so the loops may be detected as usual.  The correct fix is to
ensure that is_ctrl_altering_stmt does not return true for_builtin_prefetch,

Zdenek

Re: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Zdenek Dvorak]

Hi,

> >I would rather say it is CFG problem - for known builtins, like
> >__bultiln_prefetch we certainly don't need to produce nonlocal goto edges.
> 
> You are right in this case. But in general, a function call may indirectly goto
> the non-local labels. I didn't see any logic in gcc that handle this for loop 
> construction.

no special logic is needed; possible jumps due to non-local labels are decribed by
CFG,

Zdenek

RE: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Fang, Changpeng]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

>this does not make much sense to me; why should there be no natural loops?  Edges in CFG (conservatively)
>correctly describe the control flow, so the loops may be detected as usual.  The correct fix is to
>ensure that is_ctrl_altering_stmt does not return true for_builtin_prefetch,

You are right. We should address the problem exposed by _builtin_prefetch in this patch.
Attached is the patch that thinks _builtin_prefetch does not change the control flow.

Is this OK to commit?

Thanks,

Changpeng



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-pr44503-builtin_prefetch-does-not-change-control-flo.patch --]
[-- Type: text/x-patch; name="0001-pr44503-builtin_prefetch-does-not-change-control-flo.patch", Size: 956 bytes --]

From a5ee46bc858991c3ebe1abac680e0fe862dc98a1 Mon Sep 17 00:00:00 2001
From: Changpeng Fang <chfang@pathscale.(none)>
Date: Tue, 15 Jun 2010 14:34:09 -0700
Subject: [PATCH] pr44503 builtin_prefetch does not change control flow

	*gcc/tree-cfg.c (is_ctrl_altering_stmt): Return false if
	the gimple_call is _builtin_prefetch.
---
 gcc/tree-cfg.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index eca3ed0..76b7f56 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2259,6 +2259,10 @@ is_ctrl_altering_stmt (gimple t)
       {
 	int flags = gimple_call_flags (t);
 
+        /* BUILT_IN_PREFETCH would never alter the control flow.  */  
+	if (gimple_call_builtin_p (t, BUILT_IN_PREFETCH))
+	  return false;
+
 	/* A non-pure/const call alters flow control if the current
 	   function has nonlocal labels.  */
 	if (!(flags & (ECF_CONST | ECF_PURE)) && cfun->has_nonlocal_label)
-- 
1.6.3.3

Re: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Zdenek Dvorak]

Hi,

> >this does not make much sense to me; why should there be no natural loops?  Edges in CFG (conservatively)
> >correctly describe the control flow, so the loops may be detected as usual.  The correct fix is to
> >ensure that is_ctrl_altering_stmt does not return true for_builtin_prefetch,
> 
> You are right. We should address the problem exposed by _builtin_prefetch in this patch.
> Attached is the patch that thinks _builtin_prefetch does not change the control flow.

while a more comprehensive solution (as suggested by Honza) would be better long-term,
for now this seems ok to me (but I cannot approve it),

Zdenek

RE: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Fang, Changpeng]

-fprefetch-loop-arrays.
>
> The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
> call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop
> construction problem. When the current function has non local labels, there are no natural loops in the
> function.

>I would rather say it is CFG problem - for known builtins, like
>__bultiln_prefetch we certainly don't need to produce nonlocal goto edges.

>In fact this might be perhaps bundled in my "leaf" attribute patch I proposed
>some time ago.
 
Hi, Honza:

Would you please give a link to your leaf attribute patch ?

Thanks,

Changpeng

Re: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Jan Hubicka]

> -fprefetch-loop-arrays.
> >
> > The problem is that a non-local label (for setjmp) exists in the function. When we insert a _builtin_prefetch
> > call, this prefetch is considered potentially changing the control flow. We think this is, in general, a loop
> > construction problem. When the current function has non local labels, there are no natural loops in the
> > function.
> 
> >I would rather say it is CFG problem - for known builtins, like
> >__bultiln_prefetch we certainly don't need to produce nonlocal goto edges.
> 
> >In fact this might be perhaps bundled in my "leaf" attribute patch I proposed
> >some time ago.
>  
> Hi, Honza:
> 
> Would you please give a link to your leaf attribute patch ?

http://gcc.gnu.org/ml/gcc-patches/2010-06/msg01329.html

It allows to tag builtins to be "well behaved" that is not calling functions within current unit
and returning normally (or not at all).  I think non-local goto can be defined as not normal that
would allow cfg builder to not make edges on any leaf functions.

External function calls can do nonlocal goto back only via callback as far as I know.

Honza
> 
> Thanks,
> 
> Changpeng

RE: [PATCH, PR44503] Fixes "control flow in the middle of basic block" with -fprefetch-loop-arrays

[Fang, Changpeng]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

Resend the patch adding Diego.

Diego:

  What do you think that we consider _builtin_prefetch does not alter the control flow?
Is this patch OK to commit now?

Thanks,

Changpeng



________________________________________
From: Zdenek Dvorak [rakdver@kam.mff.cuni.cz]
Sent: Tuesday, June 15, 2010 10:48 PM
To: Fang, Changpeng
Cc: gcc-patches@gcc.gnu.org; rguenther@suse.de; sebpop@gmail.com
Subject: Re: [PATCH, PR44503] Fixes "control flow in the middle of basic        block" with -fprefetch-loop-arrays

Hi,

> >this does not make much sense to me; why should there be no natural loops?  Edges in CFG (conservatively)
> >correctly describe the control flow, so the loops may be detected as usual.  The correct fix is to
> >ensure that is_ctrl_altering_stmt does not return true for_builtin_prefetch,
>
> You are right. We should address the problem exposed by _builtin_prefetch in this patch.
> Attached is the patch that thinks _builtin_prefetch does not change the control flow.

while a more comprehensive solution (as suggested by Honza) would be better long-term,
for now this seems ok to me (but I cannot approve it),

Zdenek


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-pr44503-builtin_prefetch-does-not-change-control-flo.patch --]
[-- Type: text/x-patch; name="0001-pr44503-builtin_prefetch-does-not-change-control-flo.patch", Size: 956 bytes --]

From a5ee46bc858991c3ebe1abac680e0fe862dc98a1 Mon Sep 17 00:00:00 2001
From: Changpeng Fang <chfang@pathscale.(none)>
Date: Tue, 15 Jun 2010 14:34:09 -0700
Subject: [PATCH] pr44503 builtin_prefetch does not change control flow

	*gcc/tree-cfg.c (is_ctrl_altering_stmt): Return false if
	the gimple_call is _builtin_prefetch.
---
 gcc/tree-cfg.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index eca3ed0..76b7f56 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2259,6 +2259,10 @@ is_ctrl_altering_stmt (gimple t)
       {
 	int flags = gimple_call_flags (t);
 
+        /* BUILT_IN_PREFETCH would never alter the control flow.  */  
+	if (gimple_call_builtin_p (t, BUILT_IN_PREFETCH))
+	  return false;
+
 	/* A non-pure/const call alters flow control if the current
 	   function has nonlocal labels.  */
 	if (!(flags & (ECF_CONST | ECF_PURE)) && cfun->has_nonlocal_label)
-- 
1.6.3.3