diff mbox series

Restore RTL alias analysis for hard frame pointer

Message ID 1930502.usQuhbGJ8B@fomalhaut
State New
Headers show
Series Restore RTL alias analysis for hard frame pointer | expand

Commit Message

Eric Botcazou Oct. 28, 2022, 9:10 a.m. UTC 
Hi,

the following change:

2021-07-28  Bin Cheng  <bin.cheng@linux.alibaba.com>

	* alias.c (init_alias_analysis): Don't skip prologue/epilogue.

broke the alias analysis for the hard frame pointer (when it is used as a 
frame pointer, i.e. when the frame pointer is not eliminated) described in the 
large comment at the top of the file, because static_reg_base_value is set for 
it and, consequently, new_reg_base_value too.  So when the instruction saving 
the stack pointer into the hard frame pointer in the prologue is processed, it 
is viewed as a second set of the hard frame pointer and to a different value 
by record_set, which then resets new_reg_base_value to 0 and the game is over.

This e.g. hampers the performance of the var-tracking RTL pass for parameters 
passed on the stack like on x86, leading to regressions when debugging, but 
code generation is very likely affected too.

Bootstrapped/regtested on x86-64/Linux, OK for mainline and 12 branch?


2022-10-28  Eric Botcazou  <ebotcazou@adacore.com>

	* alias.cc (init_alias_analysis): Do not record sets to the hard
	frame pointer if the frame pointer has not been eliminated.

Comments

Richard Biener Oct. 28, 2022, 12:27 p.m. UTC | #1 
On Fri, Oct 28, 2022 at 11:11 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> the following change:
>
> 2021-07-28  Bin Cheng  <bin.cheng@linux.alibaba.com>
>
>         * alias.c (init_alias_analysis): Don't skip prologue/epilogue.
>
> broke the alias analysis for the hard frame pointer (when it is used as a
> frame pointer, i.e. when the frame pointer is not eliminated) described in the
> large comment at the top of the file, because static_reg_base_value is set for
> it and, consequently, new_reg_base_value too.  So when the instruction saving
> the stack pointer into the hard frame pointer in the prologue is processed, it
> is viewed as a second set of the hard frame pointer and to a different value
> by record_set, which then resets new_reg_base_value to 0 and the game is over.
>
> This e.g. hampers the performance of the var-tracking RTL pass for parameters
> passed on the stack like on x86, leading to regressions when debugging, but
> code generation is very likely affected too.
>
> Bootstrapped/regtested on x86-64/Linux, OK for mainline and 12 branch?

OK for trunk and 12 after a while of burn-in.

Thanks,
Richard.

>
> 2022-10-28  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * alias.cc (init_alias_analysis): Do not record sets to the hard
>         frame pointer if the frame pointer has not been eliminated.
>
> --
> Eric Botcazou
Richard Biener Oct. 28, 2022, 12:27 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 2:27 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Oct 28, 2022 at 11:11 AM Eric Botcazou via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > the following change:
> >
> > 2021-07-28  Bin Cheng  <bin.cheng@linux.alibaba.com>
> >
> >         * alias.c (init_alias_analysis): Don't skip prologue/epilogue.
> >
> > broke the alias analysis for the hard frame pointer (when it is used as a
> > frame pointer, i.e. when the frame pointer is not eliminated) described in the
> > large comment at the top of the file, because static_reg_base_value is set for
> > it and, consequently, new_reg_base_value too.  So when the instruction saving
> > the stack pointer into the hard frame pointer in the prologue is processed, it
> > is viewed as a second set of the hard frame pointer and to a different value
> > by record_set, which then resets new_reg_base_value to 0 and the game is over.
> >
> > This e.g. hampers the performance of the var-tracking RTL pass for parameters
> > passed on the stack like on x86, leading to regressions when debugging, but
> > code generation is very likely affected too.
> >
> > Bootstrapped/regtested on x86-64/Linux, OK for mainline and 12 branch?
>
> OK for trunk and 12 after a while of burn-in.

Oh, do you have a testcase suitable for the testsuite?

> Thanks,
> Richard.
>
> >
> > 2022-10-28  Eric Botcazou  <ebotcazou@adacore.com>
> >
> >         * alias.cc (init_alias_analysis): Do not record sets to the hard
> >         frame pointer if the frame pointer has not been eliminated.
> >
> > --
> > Eric Botcazou
Eric Botcazou Oct. 29, 2022, 8:19 a.m. UTC | #3 
> OK for trunk and 12 after a while of burn-in.

Thanks!

> Oh, do you have a testcase suitable for the testsuite?

I only have an Ada testcase for GDB, let me try to find something more usable.
Richard Biener Oct. 29, 2022, 11:03 a.m. UTC | #4 
> Am 29.10.2022 um 10:19 schrieb Eric Botcazou <botcazou@adacore.com>:
> 
> 
>> 
>> OK for trunk and 12 after a while of burn-in.
> 
> Thanks!
> 
>> Oh, do you have a testcase suitable for the testsuite?
> 
> I only have an Ada testcase for GDB, let me try to find something more usable.

Not too bad if it works to scan-assembler for the DWARF?

> -- 
> Eric Botcazou
> 
>
Eric Botcazou Nov. 9, 2022, 9:47 a.m. UTC | #5 
> Oh, do you have a testcase suitable for the testsuite?

C guality testcase attached, it fails on x86/Linux with -m32 on the gcc-12 
branch (which does not have the fix):

FAIL: gcc.dg/guality/param-6.c   -O1  -DPREVENT_OPTIMIZATION  line 15 i == 5
FAIL: gcc.dg/guality/param-6.c   -O2  -DPREVENT_OPTIMIZATION  line 15 i == 5
FAIL: gcc.dg/guality/param-6.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 15 i == 
5
FAIL: gcc.dg/guality/param-6.c   -Os  -DPREVENT_OPTIMIZATION  line 15 i == 5
FAIL: gcc.dg/guality/param-6.c   -O2 -flto -fno-use-linker-plugin -flto-
partition=none  -DPREVENT_OPTIMIZATION line 15 i == 5
FAIL: gcc.dg/guality/param-6.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-
objects  -DPREVENT_OPTIMIZATION line 15 i == 5

but passes on the mainline (which has the fix) and on the gcc-11 branch (which 
does not have the issue).  The fix also eliminates other regressions:

-FAIL: gcc.dg/guality/drap.c   -Os  -DPREVENT_OPTIMIZATION  line 21 a == 5
-FAIL: gcc.dg/guality/drap.c   -Os  -DPREVENT_OPTIMIZATION  line 22 b == 6
-FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 35 v == 
1
-FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 36 e == 
&a[1]
-FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 40 v == 
1
-FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 41 e == 
&a[1]
-FAIL: gcc.dg/guality/pr43177.c   -Os  -DPREVENT_OPTIMIZATION  line 15 x == 7
-FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 20 y == 
25
-FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 20 z == 
6
-FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 23 y == 
117
-FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 23 z == 
8
-FAIL: gcc.dg/guality/pr54519-4.c   -Os  -DPREVENT_OPTIMIZATION  line 17 y == 
25
-FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 a == 5
-FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 b == 6
-FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 c == 5
-FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 a == 5
-FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 b == 6
-FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 c == 5
-FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 a == 
5
-FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 b == 
6
-FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 c == 
5
-FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 a == 5
-FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 b == 6
-FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 c == 5
-FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
partition=none  -DPREVENT_OPTIMIZATION line 17 a == 5
-FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
partition=none  -DPREVENT_OPTIMIZATION line 17 b == 6
-FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
partition=none  -DPREVENT_OPTIMIZATION line 17 c == 5
-FAIL: gcc.dg/guality/sra-1.c   -Os  -DPREVENT_OPTIMIZATION  line 43 a.j == 14
-FAIL: gcc.dg/guality/vla-1.c   -O1  -DPREVENT_OPTIMIZATION  line 24 i == 5
-FAIL: gcc.dg/guality/vla-1.c   -O2  -DPREVENT_OPTIMIZATION  line 24 i == 5
-FAIL: gcc.dg/guality/vla-1.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 24 i == 5
-FAIL: gcc.dg/guality/vla-1.c   -Os  -DPREVENT_OPTIMIZATION  line 24 i == 5
-FAIL: gcc.dg/guality/vla-1.c   -O2 -flto -fno-use-linker-plugin -flto-
partition=none  -DPREVENT_OPTIMIZATION line 24 i == 5

present on the gcc-12 branch wrt the gcc-11 branch (apparently nobody really 
cares about the guality testsuite on x86/Linux).


	* gcc.dg/guality/param-6.c: New test.
Richard Biener Nov. 9, 2022, 10:44 a.m. UTC | #6 
> Am 09.11.2022 um 10:49 schrieb Eric Botcazou <botcazou@adacore.com>:
> 
> 
>> 
>> Oh, do you have a testcase suitable for the testsuite?
> 
> C guality testcase attached, it fails on x86/Linux with -m32 on the gcc-12 
> branch (which does not have the fix):
> 
> FAIL: gcc.dg/guality/param-6.c   -O1  -DPREVENT_OPTIMIZATION  line 15 i == 5
> FAIL: gcc.dg/guality/param-6.c   -O2  -DPREVENT_OPTIMIZATION  line 15 i == 5
> FAIL: gcc.dg/guality/param-6.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 15 i == 
> 5
> FAIL: gcc.dg/guality/param-6.c   -Os  -DPREVENT_OPTIMIZATION  line 15 i == 5
> FAIL: gcc.dg/guality/param-6.c   -O2 -flto -fno-use-linker-plugin -flto-
> partition=none  -DPREVENT_OPTIMIZATION line 15 i == 5
> FAIL: gcc.dg/guality/param-6.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-
> objects  -DPREVENT_OPTIMIZATION line 15 i == 5
> 
> but passes on the mainline (which has the fix) and on the gcc-11 branch (which 
> does not have the issue).  The fix also eliminates other regressions:
> 
> -FAIL: gcc.dg/guality/drap.c   -Os  -DPREVENT_OPTIMIZATION  line 21 a == 5
> -FAIL: gcc.dg/guality/drap.c   -Os  -DPREVENT_OPTIMIZATION  line 22 b == 6
> -FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 35 v == 
> 1
> -FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 36 e == 
> &a[1]
> -FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 40 v == 
> 1
> -FAIL: gcc.dg/guality/pr43051-1.c   -Os  -DPREVENT_OPTIMIZATION  line 41 e == 
> &a[1]
> -FAIL: gcc.dg/guality/pr43177.c   -Os  -DPREVENT_OPTIMIZATION  line 15 x == 7
> -FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 20 y == 
> 25
> -FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 20 z == 
> 6
> -FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 23 y == 
> 117
> -FAIL: gcc.dg/guality/pr54519-3.c   -Os  -DPREVENT_OPTIMIZATION  line 23 z == 
> 8
> -FAIL: gcc.dg/guality/pr54519-4.c   -Os  -DPREVENT_OPTIMIZATION  line 17 y == 
> 25
> -FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 a == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 b == 6
> -FAIL: gcc.dg/guality/pr54796.c   -O1  -DPREVENT_OPTIMIZATION  line 17 c == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 a == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 b == 6
> -FAIL: gcc.dg/guality/pr54796.c   -O2  -DPREVENT_OPTIMIZATION  line 17 c == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 a == 
> 5
> -FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 b == 
> 6
> -FAIL: gcc.dg/guality/pr54796.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 17 c == 
> 5
> -FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 a == 5
> -FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 b == 6
> -FAIL: gcc.dg/guality/pr54796.c   -Os  -DPREVENT_OPTIMIZATION  line 17 c == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
> partition=none  -DPREVENT_OPTIMIZATION line 17 a == 5
> -FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
> partition=none  -DPREVENT_OPTIMIZATION line 17 b == 6
> -FAIL: gcc.dg/guality/pr54796.c   -O2 -flto -fno-use-linker-plugin -flto-
> partition=none  -DPREVENT_OPTIMIZATION line 17 c == 5
> -FAIL: gcc.dg/guality/sra-1.c   -Os  -DPREVENT_OPTIMIZATION  line 43 a.j == 14
> -FAIL: gcc.dg/guality/vla-1.c   -O1  -DPREVENT_OPTIMIZATION  line 24 i == 5
> -FAIL: gcc.dg/guality/vla-1.c   -O2  -DPREVENT_OPTIMIZATION  line 24 i == 5
> -FAIL: gcc.dg/guality/vla-1.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 24 i == 5
> -FAIL: gcc.dg/guality/vla-1.c   -Os  -DPREVENT_OPTIMIZATION  line 24 i == 5
> -FAIL: gcc.dg/guality/vla-1.c   -O2 -flto -fno-use-linker-plugin -flto-
> partition=none  -DPREVENT_OPTIMIZATION line 24 i == 5
> 
> present on the gcc-12 branch wrt the gcc-11 branch (apparently nobody really 
> cares about the guality testsuite on x86/Linux).

Thanks a lot, OK.

Richard 

> 
> 
>    * gcc.dg/guality/param-6.c: New test.
> 
> -- 
> Eric Botcazou
> <param-6.c>
diff mbox series

Patch

diff --git a/gcc/alias.cc b/gcc/alias.cc
index d54feb15268..c62837dd854 100644
--- a/gcc/alias.cc
+++ b/gcc/alias.cc
@@ -3369,6 +3369,10 @@  memory_modified_in_insn_p (const_rtx mem, const_rtx insn)
 void
 init_alias_analysis (void)
 {
+  const bool frame_pointer_eliminated
+    = reload_completed
+      && !frame_pointer_needed
+      && targetm.can_eliminate (FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM);
   unsigned int maxreg = max_reg_num ();
   int changed, pass;
   int i;
@@ -3446,12 +3450,8 @@  init_alias_analysis (void)
       for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
 	if (static_reg_base_value[i]
 	    /* Don't treat the hard frame pointer as special if we
-	       eliminated the frame pointer to the stack pointer instead.  */
-	    && !(i == HARD_FRAME_POINTER_REGNUM
-		 && reload_completed
-		 && !frame_pointer_needed
-		 && targetm.can_eliminate (FRAME_POINTER_REGNUM,
-					   STACK_POINTER_REGNUM)))
+	       eliminated the frame pointer to the stack pointer.  */
+	    && !(i == HARD_FRAME_POINTER_REGNUM && frame_pointer_eliminated))
 	  {
 	    new_reg_base_value[i] = static_reg_base_value[i];
 	    bitmap_set_bit (reg_seen, i);
@@ -3467,10 +3467,15 @@  init_alias_analysis (void)
 		{
 		  rtx note, set;
 
+		  /* Treat the hard frame pointer as special unless we
+		     eliminated the frame pointer to the stack pointer.  */
+		  if (!frame_pointer_eliminated
+		      && modified_in_p (hard_frame_pointer_rtx, insn))
+		    continue;
+
 		  /* If this insn has a noalias note, process it,  Otherwise,
 		     scan for sets.  A simple set will have no side effects
 		     which could change the base value of any other register.  */
-
 		  if (GET_CODE (PATTERN (insn)) == SET
 		      && REG_NOTES (insn) != 0
 		      && find_reg_note (insn, REG_NOALIAS, NULL_RTX))