Re: [PATCH] i386: Replace frame pointer with stack pointer in debug insns

[Uros Bizjak]

On Sun, Aug 13, 2017 at 7:35 PM, H.J. Lu  wrote:
> On Sun, Aug 13, 2017 at 9:15 AM, Uros Bizjak  wrote:
>> On Sat, Aug 12, 2017 at 3:32 AM, H.J. Lu  wrote:
>>> When we eliminate frame pointer, we should also replace frame pointer
>>> with stack pointer - UNITS_PER_WORD in debug insns.  This patch fixed:
>>>
>>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b1 == 9
>>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b2 == 73
>>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b3 == 585
>>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b4 == 4681
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.f == 5.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.g == 6.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s2.g == 6.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.f == 5.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.g == 6.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.f == 5.0
>>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.g == 6.0
>>>
>>> on Linux/i386.
>>>
>>> Tested on i686 and x86-64.  OK for trunk?

OK wit ha small comment adjustment below.

Thanks,
Uros.

>>> H.J.
>>> ---
>>>
>>> PR target/81820
>>> * config/i386/i386.c (ix86_finalize_stack_frame_flags): Replace
>>> frame pointer with stack pointer - UNITS_PER_WORD in debug insns.
>>> ---
>>>  gcc/config/i386/i386.c | 36 
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>>> index b04321a8d40..0094f2c4441 100644
>>> --- a/gcc/config/i386/i386.c
>>> +++ b/gcc/config/i386/i386.c
>>> @@ -14281,6 +14281,42 @@ ix86_finalize_stack_frame_flags (void)
>>>df_scan_blocks ();
>>>df_compute_regs_ever_live (true);
>>>df_analyze ();
>>> +
>>> +  if (flag_var_tracking)
>>> +   {
>>> + /* Since frame pointer is no longer needed, replace it with
>>> +stack pointer - UNITS_PER_WORD in debug insns.  */

... Since frame pointer is no longer available, ...

>>> + df_ref ref, next;
>>> + for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
>>> +  ref; ref = next)
>>> +   {
>>> + rtx_insn *insn = DF_REF_INSN (ref);
>>> + /* Make sure the next ref is for a different instruction,
>>> +so that we're not affected by the rescan.  */
>>> + next = DF_REF_NEXT_REG (ref);
>>> + while (next && DF_REF_INSN (next) == insn)
>>> +   next = DF_REF_NEXT_REG (next);
>>
>> Can you please explain why the above loop is needed?
>>
>
> After df_insn_rescan below is called, DF_REF_NEXT_REG (ref)
> may point to the current debug instruction we just updated.
> gcc.dg/guality/pr58791-5.c is such an example.  If I take out the loop:
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 0094f2c4441..cef3a6e4816 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -14294,8 +14294,6 @@ ix86_finalize_stack_frame_flags (void)
>  /* Make sure the next ref is for a different instruction,
>so that we're not affected by the rescan.  */
>  next = DF_REF_NEXT_REG (ref);
> -while (next && DF_REF_INSN (next) == insn)
> - next = DF_REF_NEXT_REG (next);
>
>  if (DEBUG_INSN_P (insn))
>   {
>
> I got:
>
> [hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -m32 -Os -g
> /export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c
> during RTL pass: pro_and_epilogue
> /export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c:
> In function ‘foo’:
> /export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c:22:1:
> internal compiler error: Segmentation fault
>  }
>  ^
> 0xf0cae1 crash_signal
> /export/gnu/import/git/sources/gcc/gcc/toplev.c:341
> 0x1329df6 ix86_finalize_stack_frame_flags
> /export/gnu/import/git/sources/gcc/gcc/config/i386/i386.c:14293
> 0x132a5be ix86_expand_prologue()
> /export/gnu/import/git/sources/gcc/gcc/config/i386/i386.c:14440
> 0x165edaf gen_prologue()
> /export/gnu/import/git/sources/gcc/gcc/config/i386/i386.md:12464
> 0x130c25d target_gen_prologue
> /export/gnu/import/git/sources/gcc/gcc/config/i386/i386.md:18659
> 0xb30b5e make_prologue_seq
> /export/gnu/import/git/sources/gcc/gcc/function.c:5835
> 0xb30d3e thread_prologue_and_epilogue_insns()
> /export/gnu/import/git/sources/gcc/gcc/function.c:5952
> 0xb31d46 rest_of_handle_thread_prologue_and_epilogue
> /export/gnu/import/git/sources/gcc/gcc/function.c:6443
> 0xb31dc8 execute
> /export/gnu/import/git/sources/gcc/gcc/function.c:6485
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See  for instructions.
> 

Re: [PATCH] i386: Replace frame pointer with stack pointer in debug insns

[H.J. Lu]

On Sun, Aug 13, 2017 at 9:15 AM, Uros Bizjak  wrote:
> On Sat, Aug 12, 2017 at 3:32 AM, H.J. Lu  wrote:
>> When we eliminate frame pointer, we should also replace frame pointer
>> with stack pointer - UNITS_PER_WORD in debug insns.  This patch fixed:
>>
>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b1 == 9
>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b2 == 73
>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b3 == 585
>> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b4 == 4681
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.f == 5.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.g == 6.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s2.g == 6.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.f == 5.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.g == 6.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.f == 5.0
>> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.g == 6.0
>>
>> on Linux/i386.
>>
>> Tested on i686 and x86-64.  OK for trunk?
>>
>> H.J.
>> ---
>>
>> PR target/81820
>> * config/i386/i386.c (ix86_finalize_stack_frame_flags): Replace
>> frame pointer with stack pointer - UNITS_PER_WORD in debug insns.
>> ---
>>  gcc/config/i386/i386.c | 36 
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index b04321a8d40..0094f2c4441 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -14281,6 +14281,42 @@ ix86_finalize_stack_frame_flags (void)
>>df_scan_blocks ();
>>df_compute_regs_ever_live (true);
>>df_analyze ();
>> +
>> +  if (flag_var_tracking)
>> +   {
>> + /* Since frame pointer is no longer needed, replace it with
>> +stack pointer - UNITS_PER_WORD in debug insns.  */
>> + df_ref ref, next;
>> + for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
>> +  ref; ref = next)
>> +   {
>> + rtx_insn *insn = DF_REF_INSN (ref);
>> + /* Make sure the next ref is for a different instruction,
>> +so that we're not affected by the rescan.  */
>> + next = DF_REF_NEXT_REG (ref);
>> + while (next && DF_REF_INSN (next) == insn)
>> +   next = DF_REF_NEXT_REG (next);
>
> Can you please explain why the above loop is needed?
>

After df_insn_rescan below is called, DF_REF_NEXT_REG (ref)
may point to the current debug instruction we just updated.
gcc.dg/guality/pr58791-5.c is such an example.  If I take out the loop:

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 0094f2c4441..cef3a6e4816 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -14294,8 +14294,6 @@ ix86_finalize_stack_frame_flags (void)
 /* Make sure the next ref is for a different instruction,
   so that we're not affected by the rescan.  */
 next = DF_REF_NEXT_REG (ref);
-while (next && DF_REF_INSN (next) == insn)
- next = DF_REF_NEXT_REG (next);

 if (DEBUG_INSN_P (insn))
  {

I got:

[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -m32 -Os -g
/export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c
during RTL pass: pro_and_epilogue
/export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c:
In function ‘foo’:
/export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/guality/pr58791-5.c:22:1:
internal compiler error: Segmentation fault
 }
 ^
0xf0cae1 crash_signal
/export/gnu/import/git/sources/gcc/gcc/toplev.c:341
0x1329df6 ix86_finalize_stack_frame_flags
/export/gnu/import/git/sources/gcc/gcc/config/i386/i386.c:14293
0x132a5be ix86_expand_prologue()
/export/gnu/import/git/sources/gcc/gcc/config/i386/i386.c:14440
0x165edaf gen_prologue()
/export/gnu/import/git/sources/gcc/gcc/config/i386/i386.md:12464
0x130c25d target_gen_prologue
/export/gnu/import/git/sources/gcc/gcc/config/i386/i386.md:18659
0xb30b5e make_prologue_seq
/export/gnu/import/git/sources/gcc/gcc/function.c:5835
0xb30d3e thread_prologue_and_epilogue_insns()
/export/gnu/import/git/sources/gcc/gcc/function.c:5952
0xb31d46 rest_of_handle_thread_prologue_and_epilogue
/export/gnu/import/git/sources/gcc/gcc/function.c:6443
0xb31dc8 execute
/export/gnu/import/git/sources/gcc/gcc/function.c:6485
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
[hjl@gnu-tools-1 gcc]$


> Thanks,
> Uros.
>
>> + if (DEBUG_INSN_P (insn))
>> +   {
>> + bool changed = false;
>> + for (; ref != next; ref = DF_REF_NEXT_REG (ref))
>> +   {
>> + rtx *loc = DF_REF_LOC (ref);
>> + if (*loc == 

Re: [PATCH] i386: Replace frame pointer with stack pointer in debug insns

[Uros Bizjak]

On Sat, Aug 12, 2017 at 3:32 AM, H.J. Lu  wrote:
> When we eliminate frame pointer, we should also replace frame pointer
> with stack pointer - UNITS_PER_WORD in debug insns.  This patch fixed:
>
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b1 == 9
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b2 == 73
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b3 == 585
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b4 == 4681
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s2.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.g == 6.0
>
> on Linux/i386.
>
> Tested on i686 and x86-64.  OK for trunk?
>
> H.J.
> ---
>
> PR target/81820
> * config/i386/i386.c (ix86_finalize_stack_frame_flags): Replace
> frame pointer with stack pointer - UNITS_PER_WORD in debug insns.
> ---
>  gcc/config/i386/i386.c | 36 
>  1 file changed, 36 insertions(+)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index b04321a8d40..0094f2c4441 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -14281,6 +14281,42 @@ ix86_finalize_stack_frame_flags (void)
>df_scan_blocks ();
>df_compute_regs_ever_live (true);
>df_analyze ();
> +
> +  if (flag_var_tracking)
> +   {
> + /* Since frame pointer is no longer needed, replace it with
> +stack pointer - UNITS_PER_WORD in debug insns.  */
> + df_ref ref, next;
> + for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
> +  ref; ref = next)
> +   {
> + rtx_insn *insn = DF_REF_INSN (ref);
> + /* Make sure the next ref is for a different instruction,
> +so that we're not affected by the rescan.  */
> + next = DF_REF_NEXT_REG (ref);
> + while (next && DF_REF_INSN (next) == insn)
> +   next = DF_REF_NEXT_REG (next);

Can you please explain why the above loop is needed?

Thanks,
Uros.

> + if (DEBUG_INSN_P (insn))
> +   {
> + bool changed = false;
> + for (; ref != next; ref = DF_REF_NEXT_REG (ref))
> +   {
> + rtx *loc = DF_REF_LOC (ref);
> + if (*loc == hard_frame_pointer_rtx)
> +   {
> + *loc = plus_constant (Pmode,
> +   stack_pointer_rtx,
> +   -UNITS_PER_WORD);
> + changed = true;
> +   }
> +   }
> + if (changed)
> +   df_insn_rescan (insn);
> +   }
> +   }
> +   }
> +
>recompute_frame_layout_p = true;
>  }
>
> --
> 2.13.4
>

Re: [PATCH] i386: Replace frame pointer with stack pointer in debug insns

[Uros Bizjak]

On Sat, Aug 12, 2017 at 3:32 AM, H.J. Lu  wrote:
> When we eliminate frame pointer, we should also replace frame pointer
> with stack pointer - UNITS_PER_WORD in debug insns.  This patch fixed:
>
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b1 == 9
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b2 == 73
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b3 == 585
> FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b4 == 4681
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s2.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.g == 6.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.f == 5.0
> FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.g == 6.0
>
> on Linux/i386.
>
> Tested on i686 and x86-64.  OK for trunk?

Is it possible to detect when frame pointer setup was omitted and do
the replacement only in that case? Do we have to do the replacement
unconditinally due to shrink-wrapping and similar passes?

Uros.

> H.J.
> ---
>
> PR target/81820
> * config/i386/i386.c (ix86_finalize_stack_frame_flags): Replace
> frame pointer with stack pointer - UNITS_PER_WORD in debug insns.
> ---
>  gcc/config/i386/i386.c | 36 
>  1 file changed, 36 insertions(+)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index b04321a8d40..0094f2c4441 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -14281,6 +14281,42 @@ ix86_finalize_stack_frame_flags (void)
>df_scan_blocks ();
>df_compute_regs_ever_live (true);
>df_analyze ();
> +
> +  if (flag_var_tracking)
> +   {
> + /* Since frame pointer is no longer needed, replace it with
> +stack pointer - UNITS_PER_WORD in debug insns.  */
> + df_ref ref, next;
> + for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
> +  ref; ref = next)
> +   {
> + rtx_insn *insn = DF_REF_INSN (ref);
> + /* Make sure the next ref is for a different instruction,
> +so that we're not affected by the rescan.  */
> + next = DF_REF_NEXT_REG (ref);
> + while (next && DF_REF_INSN (next) == insn)
> +   next = DF_REF_NEXT_REG (next);
> +
> + if (DEBUG_INSN_P (insn))
> +   {
> + bool changed = false;
> + for (; ref != next; ref = DF_REF_NEXT_REG (ref))
> +   {
> + rtx *loc = DF_REF_LOC (ref);
> + if (*loc == hard_frame_pointer_rtx)
> +   {
> + *loc = plus_constant (Pmode,
> +   stack_pointer_rtx,
> +   -UNITS_PER_WORD);
> + changed = true;
> +   }
> +   }
> + if (changed)
> +   df_insn_rescan (insn);
> +   }
> +   }
> +   }
> +
>recompute_frame_layout_p = true;
>  }
>
> --
> 2.13.4
>

[PATCH] i386: Replace frame pointer with stack pointer in debug insns

[H.J. Lu]

When we eliminate frame pointer, we should also replace frame pointer
with stack pointer - UNITS_PER_WORD in debug insns.  This patch fixed:

FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b1 == 9
FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b2 == 73
FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b3 == 585
FAIL: gcc.dg/guality/pr58791-5.c   -Os  line pr58791-5.c:20 b4 == 4681
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.f == 5.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s1.g == 6.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:17 s2.g == 6.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.f == 5.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s1.g == 6.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.f == 5.0
FAIL: gcc.dg/guality/pr59776.c   -Os  line pr59776.c:20 s2.g == 6.0

on Linux/i386.

Tested on i686 and x86-64.  OK for trunk?

H.J.
---

PR target/81820
* config/i386/i386.c (ix86_finalize_stack_frame_flags): Replace
frame pointer with stack pointer - UNITS_PER_WORD in debug insns.
---
 gcc/config/i386/i386.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b04321a8d40..0094f2c4441 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -14281,6 +14281,42 @@ ix86_finalize_stack_frame_flags (void)
   df_scan_blocks ();
   df_compute_regs_ever_live (true);
   df_analyze ();
+
+  if (flag_var_tracking)
+   {
+ /* Since frame pointer is no longer needed, replace it with
+stack pointer - UNITS_PER_WORD in debug insns.  */
+ df_ref ref, next;
+ for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
+  ref; ref = next)
+   {
+ rtx_insn *insn = DF_REF_INSN (ref);
+ /* Make sure the next ref is for a different instruction,
+so that we're not affected by the rescan.  */
+ next = DF_REF_NEXT_REG (ref);
+ while (next && DF_REF_INSN (next) == insn)
+   next = DF_REF_NEXT_REG (next);
+
+ if (DEBUG_INSN_P (insn))
+   {
+ bool changed = false;
+ for (; ref != next; ref = DF_REF_NEXT_REG (ref))
+   {
+ rtx *loc = DF_REF_LOC (ref);
+ if (*loc == hard_frame_pointer_rtx)
+   {
+ *loc = plus_constant (Pmode,
+   stack_pointer_rtx,
+   -UNITS_PER_WORD);
+ changed = true;
+   }
+   }
+ if (changed)
+   df_insn_rescan (insn);
+   }
+   }
+   }
+
   recompute_frame_layout_p = true;
 }
 
-- 
2.13.4