[PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Chen Gang]

From fadd991f87dbd5752e9b6a2cce300dfe7cc8af25 Mon Sep 17 00:00:00 2001
From: Chen Gang <gang.chen.5i5j@gmail.com>
Date: Sun, 11 Oct 2015 22:59:35 +0800
Subject: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

For some architectures (e.g. bfin), when this case occurs, they will use
another instructions instead of frame pointer (e.g. LINK for bfin), so
they can still generate correct output assembly code.

2015-10-11  Chen Gang  <gang.chen.5i5j@gmail.com>

	gcc/
	* config.in: Add HAVE_FP_INSTEAD_INSNS.
	* configure: Check HAVE_FP_INSTEAD_INSNS to set 0 or 1.
	* ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is
	needed and as global register.
---
 gcc/config.in |  5 +++++
 gcc/configure | 18 ++++++++++++++++++
 gcc/ira.c     |  3 ++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/gcc/config.in b/gcc/config.in
index 093478c..97f5957 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -1154,6 +1154,11 @@
 #undef HAVE_FWRITE_UNLOCKED
 #endif
 
+/* Define 0/1 if your machine supports frame pointer instead instructions. */
+#ifndef USED_FOR_TARGET
+#undef HAVE_FP_INSTEAD_INSNS
+#endif
+
 
 /* Define if your assembler supports specifying the alignment of objects
    allocated using the GAS .comm command. */
diff --git a/gcc/configure b/gcc/configure
index f6ae9906..a1aa430 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -22356,6 +22356,24 @@ fi
 { $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_as_flags">&5
 $as_echo "$gcc_cv_as_flags">&6; }
 
+# Check if the target have frame pointer instead instructions
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking target frame pointer instead instructions">&5
+$as_echo_n "checking target frame pointer instead instructions...">&6; }
+case "$cpu_type" in
+  bfin)
+    gcc_cv_fp_instead="yes"
+    ;;
+  *)
+    gcc_cv_fp_instead="no"
+    ;;
+esac
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_fp_instead">&5
+$as_echo "$gcc_cv_fp_instead">&6; }
+
+cat>>confdefs.h <<_ACEOF
+#define HAVE_FP_INSTEAD_INSNS `if test $gcc_cv_fp_instead = yes; then echo 1; else echo 0; fi`
+_ACEOF
+
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking assembler for .balign and .p2align">&5
 $as_echo_n "checking assembler for .balign and .p2align... ">&6; }
 if test "${gcc_cv_as_balign_and_p2align+set}" = set; then :
diff --git a/gcc/ira.c b/gcc/ira.c
index 28517c1..998d11b 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5471,7 +5471,8 @@ do_reload (void)
      register.  Often we can get away with letting the user appropriate
      the frame pointer, but we should let them know when code generation
      makes that impossible.  */
-  if (global_regs[HARD_FRAME_POINTER_REGNUM] && frame_pointer_needed)
+  if (global_regs[HARD_FRAME_POINTER_REGNUM] && frame_pointer_needed
+      && !HAVE_FP_INSTEAD_INSNS)
     {
       tree decl = global_regs_decl[HARD_FRAME_POINTER_REGNUM];
       error_at (DECL_SOURCE_LOCATION (current_function_decl),
-- 
1.9.3

 		 	   		  

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Bernd Schmidt]

On 10/11/2015 05:16 PM, Chen Gang wrote:
> For some architectures (e.g. bfin), when this case occurs, they will use
> another instructions instead of frame pointer (e.g. LINK for bfin), so
> they can still generate correct output assembly code.

What is "this case"? I don't think you have explained the problem you 
are trying to solve.

> 2015-10-11  Chen Gang  <gang.chen.5i5j@gmail.com>
>
> 	gcc/
> 	* config.in: Add HAVE_FP_INSTEAD_INSNS.
> 	* configure: Check HAVE_FP_INSTEAD_INSNS to set 0 or 1.

And of course, that should not be a configure check. If at all, use a 
target hook.


Bernd

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Chen Gang]

On 10/12/15 18:49, Bernd Schmidt wrote:
> On 10/11/2015 05:16 PM, Chen Gang wrote:
>> For some architectures (e.g. bfin), when this case occurs, they will use
>> another instructions instead of frame pointer (e.g. LINK for bfin), so
>> they can still generate correct output assembly code.
> 
> What is "this case"? I don't think you have explained the problem you are trying to solve.
> 

It is about Bug65804. I found it when building Linux bfin kernel, and
the original old version gcc can build kernel successfully.

But since the git commit "e52beba PR debug/54694", it will be failed: it
intends to check failure during building time. But for bfin, it has LINK
insn instead of, so it is still OK, and gcc should not report failure.

>> 2015-10-11  Chen Gang  <gang.chen.5i5j@gmail.com>
>>
>>     gcc/
>>     * config.in: Add HAVE_FP_INSTEAD_INSNS.
>>     * configure: Check HAVE_FP_INSTEAD_INSNS to set 0 or 1.
> 
> And of course, that should not be a configure check. If at all, use a target hook.
> 

OK, thanks. If we really need to fix it, which target hook should I use?
(or do we need a new target hook?)


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Mike Stump]

On Oct 12, 2015, at 3:32 PM, Chen Gang <xili_gchen_5257@hotmail.com> wrote:
> 
> OK, thanks. If we really need to fix it, which target hook should I use?
> (or do we need a new target hook?)

So, the first discussion would be if it is, or is not a bug.  If it isn’t, then there is no fix.  No fix, no target hook.  So far, Bernd said not a bug.

So, I’ll note that one _can_ do this with the stack pointer, as a fixed register.
When the frame pointer is fixed, one cannot do this.

The code that does this is:

  /* Diagnose uses of the hard frame pointer when it is used as a global                                                                                                                                                   
     register.  Often we can get away with letting the user appropriate                                                                                                                                                     
     the frame pointer, but we should let them know when code generation                                                                                                                                                    
     makes that impossible.  */
  if (global_regs[HARD_FRAME_POINTER_REGNUM] && frame_pointer_needed)
    {
      tree decl = global_regs_decl[HARD_FRAME_POINTER_REGNUM];
      error_at (DECL_SOURCE_LOCATION (current_function_decl),
                "frame pointer required, but reserved");
      inform (DECL_SOURCE_LOCATION (decl), "for %qD", decl);
    }

to `fix it’, one would simple remove this chunk as misguided and fix up any code gen issues exposed.

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Chen Gang]

On 10/13/15 07:02, Mike Stump wrote:
> On Oct 12, 2015, at 3:32 PM, Chen Gang <xili_gchen_5257@hotmail.com> wrote:
>>
>> OK, thanks. If we really need to fix it, which target hook should I use?
>> (or do we need a new target hook?)
> 
> So, the first discussion would be if it is, or is not a bug.  If it isn’t, then there is no fix.  No fix, no target hook.  So far, Bernd said not a bug.
> 

OK, under the bugzilla, the maintainer treated it as expected behavior
(not a bug). For me, we need more explanation for it (why we treat it
as expected behavior).


> So, I’ll note that one _can_ do this with the stack pointer, as a fixed register.
> When the frame pointer is fixed, one cannot do this.
> 

Excuse me, I do not quite understand, could you please provide more
details?

> The code that does this is:
> 
>   /* Diagnose uses of the hard frame pointer when it is used as a global                                                                                                                                                   
>      register.  Often we can get away with letting the user appropriate                                                                                                                                                     
>      the frame pointer, but we should let them know when code generation                                                                                                                                                    
>      makes that impossible.  */
>   if (global_regs[HARD_FRAME_POINTER_REGNUM] && frame_pointer_needed)
>     {
>       tree decl = global_regs_decl[HARD_FRAME_POINTER_REGNUM];
>       error_at (DECL_SOURCE_LOCATION (current_function_decl),
>                 "frame pointer required, but reserved");
>       inform (DECL_SOURCE_LOCATION (decl), "for %qD", decl);
>     }
> 
> to `fix it’, one would simple remove this chunk as misguided and fix up any code gen issues exposed.
> 

If there were not only one issues related with it, for me, what you said
sounds reasonable to me.


Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Bernd Schmidt]

On 10/13/2015 04:50 PM, Chen Gang wrote:
> OK, under the bugzilla, the maintainer treated it as expected behavior
> (not a bug). For me, we need more explanation for it (why we treat it
> as expected behavior).

A global register is under control of the user. If the compiler uses it 
as a frame pointer, it will get clobbered outside the user's control, 
which is unexpected behaviour. Therefore, the code Mike quoted detects 
that case and issues an error, indicating that you must use 
-fomit-frame-pointer if you expect to use the frame pointer register for 
other purposes.

If you want an address on the stack there's __builtin_frame_address 
which may or may not do what was intended. The code quoted in the 
bugzilla is just invalid.

>> to `fix it’, one would simple remove this chunk as misguided and fix up any code gen issues exposed.
>>
>
> If there were not only one issues related with it, for me, what you said
> sounds reasonable to me.

That's totally the wrong thing to do as the issue is not compiler code 
generation, it's the danger of clobbering a user variable.


Bernd

Re: [PATCH] gcc/ira.c: Check !HAVE_FP_INSTEAD_INSNS when frame pointer is needed and as global register

[Chen Gang]

On 10/13/15 22:56, Bernd Schmidt wrote:
> On 10/13/2015 04:50 PM, Chen Gang wrote:
>> OK, under the bugzilla, the maintainer treated it as expected behavior
>> (not a bug). For me, we need more explanation for it (why we treat it
>> as expected behavior).
> 
> A global register is under control of the user. If the compiler uses it as a frame pointer, it will get clobbered outside the user's control, which is unexpected behaviour. Therefore, the code Mike quoted detects that case and issues an error, indicating that you must use -fomit-frame-pointer if you expect to use the frame pointer register for other purposes.
> 

OK, thanks.

> If you want an address on the stack there's __builtin_frame_address which may or may not do what was intended. The code quoted in the bugzilla is just invalid.
> 

OK, thank you very much, I shall send related kernel fix patch to kernel
mailing list.

Thanks.
-- 
Chen Gang (陈刚)

Open, share, and attitude like air, water, and life which God blessed