Patchwork [rs6000] Fix PR 50906, eh_frame and other woes

login
register
mail settings
Submitter Joseph S. Myers
Date Nov. 7, 2011, 4:10 p.m.
Message ID <Pine.LNX.4.64.1111071606580.24579@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/124123/
State New
Headers show

Comments

Joseph S. Myers - Nov. 7, 2011, 4:10 p.m.
On Mon, 7 Nov 2011, Alan Modra wrote:

> On Mon, Nov 07, 2011 at 10:53:51AM +0100, Olivier Hainque wrote:
> > Another bug we're running into here is an unwarranted move of the sp
> > restore prior to register fetches despite an attempt at preventing that
> > with a stack_tie instruction (VxWorks target).
> > 
> >   http://gcc.gnu.org/ml/gcc/2011-03/msg00123.html
> > 
> > The failure can still be exposed on mainline with a minor adjustment
> > to the C testcase quoted in the msg.
> 
> Even after revision 181056?

FWIW, when I encountered such a problem in 4.4-based tools I found I 
needed the following change to stack_tie patterns to fix it (applied to 
and rediffed against trunk, but not tested there), in addition to a 
DEFAULT_ABI == ABI_V4 check such as you added.  Given all the confusion 
about this class of bugs and whether they should be addressed in each back 
end or in a target-independent way, I didn't try to send this upstream at 
the time.

2011-04-11  Joseph Myers  <joseph@codesourcery.com>

	* config/rs6000/rs6000.md (stack_tie): Use (mem:BLK (scratch)) as
	output of SET.
	(restore_stack_block, restore_stack_nonlocal): Update UNSPEC_TIE
	patterns.
Alan Modra - Nov. 8, 2011, 12:16 a.m.
On Mon, Nov 07, 2011 at 04:10:38PM +0000, Joseph S. Myers wrote:
> FWIW, when I encountered such a problem in 4.4-based tools I found I 
> needed the following change to stack_tie patterns to fix it (applied to 
> and rediffed against trunk, but not tested there), in addition to a 
> DEFAULT_ABI == ABI_V4 check such as you added.  Given all the confusion 
> about this class of bugs and whether they should be addressed in each back 
> end or in a target-independent way, I didn't try to send this upstream at 
> the time.
> 
> 2011-04-11  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* config/rs6000/rs6000.md (stack_tie): Use (mem:BLK (scratch)) as
> 	output of SET.
> 	(restore_stack_block, restore_stack_nonlocal): Update UNSPEC_TIE
> 	patterns.

OK, so you made the stack ties conflict with all memory.  I wondered
about that too, but didn't find a testcase where it mattered.  It
would certainly be safer to use the big hammer, but on the other hand
it can restrict scheduling unnecessarily.  For example

void byref (int *a)
{
  int x;

  asm ("#%0 set here" : "=r" (x) : : "r29", "r30");
  *a = x;
}

with the existing stack ties, the write to *a happens after stack
deallocation.
Joseph S. Myers - Nov. 8, 2011, 12:30 a.m.
On Tue, 8 Nov 2011, Alan Modra wrote:

> OK, so you made the stack ties conflict with all memory.  I wondered
> about that too, but didn't find a testcase where it mattered.  It

The test I had was one of those from the various PRs for this issue.

int find_num(int i)
{
  const int arr[5] = {0, 1, 2, 3, 4};
  return arr[i];
}

(-O2 -mcpu=603).  But I don't know if it reproduces with FSF 4.4 or trunk; 
there were plenty of other local changes in the 4.4-based tree where I 
found this to be necessary.
Alan Modra - Nov. 8, 2011, 5:13 a.m.
On Tue, Nov 08, 2011 at 12:30:36AM +0000, Joseph S. Myers wrote:
> On Tue, 8 Nov 2011, Alan Modra wrote:
> 
> > OK, so you made the stack ties conflict with all memory.  I wondered
> > about that too, but didn't find a testcase where it mattered.  It
> 
> The test I had was one of those from the various PRs for this issue.
> 
> int find_num(int i)
> {
>   const int arr[5] = {0, 1, 2, 3, 4};
>   return arr[i];
> }
> 
> (-O2 -mcpu=603).  But I don't know if it reproduces with FSF 4.4 or trunk; 
> there were plenty of other local changes in the 4.4-based tree where I 
> found this to be necessary.

The testcase generates good code on current gcc-4.7, gcc-4.6, and
gcc-4.5, but fails on current gcc-4.4.  I thought I had this PR
fixed on all active branches.  :-(

It's been a while since I looked at what was happening with this
testcase, but from memory what stops sheduling over the stack_tie is
alias.c:base_alias_check.  Which relies on tracking registers to see
whether two memory accesses using different regs could alias.  Quite
possibly gcc-4.4 is deficient in this area.
Olivier Hainque - Nov. 8, 2011, 10:37 a.m.
On Nov 8, 2011, at 6:13 AM, Alan Modra wrote:

> It's been a while since I looked at what was happening with this
> testcase, but from memory what stops sheduling over the stack_tie is
> alias.c:base_alias_check.  Which relies on tracking registers to see
> whether two memory accesses using different regs could alias.  Quite
> possibly gcc-4.4 is deficient in this area.

 I re-investigated our testcase yesterday and this is indeed what 
 makes the difference. I couldn't reproduce the misbehavior on mainline,
 not because of the recent change you mentioned upthread, but because
 of an earlier one, rev 180522.

 The problem is visible on 180521, and here is what is happening (roughly):
 The testcase (sources below), compiled with -O1 -fschedule-insns2 by a
 powerpc-wrs-vxworks compiler produces in .pro_and_epilogue:

   (insn 29 22 28 2 (set (reg/f:SI 11 11 [133])
          (high:SI (symbol_ref:SI ("mysym11") ...))) ./t.c:19 408 {elf_high}
     (expr_list:REG_EQUIV (high:SI (symbol_ref:SI ("mysym11") ...)

   (insn 28 29 24 2 (set (reg/f:SI 0 0 [orig:123 p11$3 ] [123])
        (lo_sum:SI (reg/f:SI 11 11 [133]) (symbol_ref:SI ("mysym11") ) 
     (expr_list:REG_EQUIV (symbol_ref:SI ("mysym11") ...)
   ...

   (note 39 24 40 2 NOTE_INSN_EPILOGUE_BEG)

   (insn 40 39 41 2 (set (reg:SI 11 11)
        (plus:SI (reg/f:SI 31 31)
            (const_int 48 [0x30]))) ./t.c:22 -1
   ...
   (insn 43 42 44 2 (set (reg:SI 25 25)
        (mem/c:SI (plus:SI (reg:SI 11 11)
                (const_int -28 [0xffffffffffffffe4])) [0 S4 A8])) ./t.c:22 -1

   (insn/f 44 43 45 2 (set (reg/f:SI 31 31)
        (mem/c:SI (plus:SI (reg:SI 11 11)
                (const_int -4 [0xfffffffffffffffc])) [0 S4 A8])) ./t.c:22 -1
 
   (insn 45 44 46 2 (set (mem/c:BLK (reg/f:SI 1 1) [0 A8])
        (unspec:BLK [
                (mem/c:BLK (reg/f:SI 1 1) [0 A8])
            ] UNSPEC_TIE)) ./t.c:22 -1
 
    (insn/f 46 45 47 2 (set (reg/f:SI 1 1)
        (reg:SI 11 11)) ./t.c:22 -1


 insn 45 is the "stack tie" intended to prevent a move of the sp restore
 (insn 46) prior to the register restores accessing the frame (insn 43 here) 

 Most of the time, as you say, this works because the mem accesses are
 considered conflicting. In this very particular case,  we get into 
 base_alias_check with 

   x = (reg/f:SI 1 1)
   y = (plus:SI (reg:SI 11 11)
         (const_int -4 [0xfffffffffffffffc]))

 from which base_alias_check infers

   x_base = (address:SI (reg/f:SI 1 1))
   y_base = (symbol_ref:SI ("mysym11") ...)

 (Erm, _that_ doesn't sound right: r11 certainly is not a base access
  to mysym11 past insn 40)

 x_base and y_base are both != 0, and we get (still within base_alias_check)
 into
 
    /* If one address is a stack reference there can be no alias:
       stack references using different base registers do not alias,
       a stack reference can not alias a parameter, and a stack reference
       can not alias a global.  */

 Eventually, the tie and the sp restore move up together prior to the
 register restore. 

 rev 180522 changes the registers allocation so that r11 is not used in
 the function body any more.

 That r11 is considered a base to mysym11 certainly looks incorrect. Now,
 looking at the comment quoted above suggests that the current stack tie
 mechanism used in emit_epilogue (relying on mem r11 to always conflict with
 mem:blk sp) is optimistic, regardless, which Joesph's experience seem to
 confirm.

 Joseph resorted to mem:scratch to impose a strong barrier. That's certainly
 safe and I don't think the performance impact can be significant, so this
 looks like a good way out.

 I tried an approach in between here: replace the stack_tie insn by a
 frame_tie insn with frame_base_rtx as the second base. In the case above,
 with frame_base_rtx==r11, this is something like

  (set (mem/c:BLK (reg:SI 11 11) [0 A8])
        (unspec:BLK [
                (mem/c:BLK (reg:SI 11 11) [0 A8])
                (mem/c:BLK (reg/f:SI 1 1) [0 A8])

 As I mentioned upthread, I'm still unclear on a couple of aspects though.
 For example, ISTM that there are paths where we might need a tie and different
 frame base regs were used before. The conditions controlling the different
 paths are intricate so I'm not quite sure. Joseph's approach would for sure
 void the need to seek answers :)

 Olivier

--

char mysym11;
char * volatile g11;

void foo (long x)
{
  /* Force frame_pointer_needed & use of r11 as a frame_reg_rtx
     from emit_epilogue.  */
  char volatile s [x];
  s[0] = 12;

  /* Force a register save/restore.  */
  asm volatile ("" : : : "25");

  /* Try to force something like (set r11 (symref:mysym))
     in the function body.  */
  {
    register char * volatile p11 asm("11");

    p11 = &mysym11;
    g11 = p11;
  }  
}
Alan Modra - Nov. 8, 2011, 1:40 p.m.
On Tue, Nov 08, 2011 at 11:37:57AM +0100, Olivier Hainque wrote:
>  Joseph resorted to mem:scratch to impose a strong barrier. That's certainly
>  safe and I don't think the performance impact can be significant, so this
>  looks like a good way out.

I agree.  Our fancy powerpc stack deallocation barriers rely too much
on alias analysis, which as you've shown (thanks!) isn't reliable in
this instance.  Other targets just use the mem:scratch barrier.

>  I tried an approach in between here: replace the stack_tie insn by a
>  frame_tie insn with frame_base_rtx as the second base. In the case above,
>  with frame_base_rtx==r11, this is something like
> 
>   (set (mem/c:BLK (reg:SI 11 11) [0 A8])
>         (unspec:BLK [
>                 (mem/c:BLK (reg:SI 11 11) [0 A8])
>                 (mem/c:BLK (reg/f:SI 1 1) [0 A8])

Looks similar to
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30282#c15
Olivier Hainque - Nov. 9, 2011, 5:15 p.m.
On Nov 8, 2011, at 2:40 PM, Alan Modra wrote:

> On Tue, Nov 08, 2011 at 11:37:57AM +0100, Olivier Hainque wrote:
>> Joseph resorted to mem:scratch to impose a strong barrier. That's certainly
>> safe and I don't think the performance impact can be significant, so this
>> looks like a good way out.
> 
> I agree.  Our fancy powerpc stack deallocation barriers rely too much
> on alias analysis, which as you've shown (thanks!) isn't reliable in
> this instance.  Other targets just use the mem:scratch barrier.

 Right. While the instance we have at hand here involves a bug
 I think (r11 perceived as a base reg to symbol), I don't see that
 we have strong guarantees that the current assumptions hold for
 sure in absence of such a bug.
  
>> I tried an approach in between here: replace the stack_tie insn by a
>> frame_tie insn with frame_base_rtx as the second base. In the case above,
>> with frame_base_rtx==r11, this is something like
>> 
>>  (set (mem/c:BLK (reg:SI 11 11) [0 A8])
>>        (unspec:BLK [
>>                (mem/c:BLK (reg:SI 11 11) [0 A8])
>>                (mem/c:BLK (reg/f:SI 1 1) [0 A8])
> 
> Looks similar to
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30282#c15

 Exactly. The idea here is to use the same mechanism in
 the other places where emit_epilogue emits a tie, making
 sure that the tie mem access will conflict with the frame
 accesses by issuing a write from the same base register.

 Now, if we can indeed have frame accesses using different
 base registers and a tie is needed after that (needs double
 checking), well, we'll need several of them.

 We could probably abstract this away and "just" keep track
 of a set of base registers used before a tie gets emitted.

 I'm not convinced that the potential gain is worth the extra
 complexity and potential risk of running into another subtle
 subcase, with hard to track sporadic runtime failures for
 starters. I don't have numbers though.

 That's a port maintainer call, I guess ?

 Olivier
Olivier Hainque - Nov. 16, 2011, 11:54 a.m.
On Nov 9, 2011, at 18:15 , Olivier Hainque wrote:
> I'm not convinced that the potential gain is worth the extra
> complexity and potential risk of running into another subtle
> subcase, with hard to track sporadic runtime failures for
> starters. I don't have numbers though.
> 
> That's a port maintainer call, I guess ?

 David, opinion on this point ?

 My understanding is that we have two options

 1) try to preserve the current attempt at maximizing
    optimization opportunities with precise stack/frame
    tie insns, 

 2) simplify for a slightly more brutal option, with
    a strong (mem:blk scratch) barrier instead

 My feeling is that 2 would be a sensible option.
 
 Trying to get the precise insns right has caused multiple
 issues (several PRs about hard to track silent wrong code
 generated, trickier implementation), and I'm not convinced
 that the legitimate code efficiency gains are worth the 
 trouble.

 As I wrote, I don't have numbers to backup the latter point
 though.

 Thanks for your feedback,

 Olivier
David Edelsohn - Nov. 16, 2011, 3:15 p.m.
On Wed, Nov 16, 2011 at 6:54 AM, Olivier Hainque <hainque@adacore.com> wrote:
>
> On Nov 9, 2011, at 18:15 , Olivier Hainque wrote:
>> I'm not convinced that the potential gain is worth the extra
>> complexity and potential risk of running into another subtle
>> subcase, with hard to track sporadic runtime failures for
>> starters. I don't have numbers though.
>>
>> That's a port maintainer call, I guess ?
>
>  David, opinion on this point ?
>
>  My understanding is that we have two options
>
>  1) try to preserve the current attempt at maximizing
>    optimization opportunities with precise stack/frame
>    tie insns,
>
>  2) simplify for a slightly more brutal option, with
>    a strong (mem:blk scratch) barrier instead
>
>  My feeling is that 2 would be a sensible option.
>
>  Trying to get the precise insns right has caused multiple
>  issues (several PRs about hard to track silent wrong code
>  generated, trickier implementation), and I'm not convinced
>  that the legitimate code efficiency gains are worth the
>  trouble.
>
>  As I wrote, I don't have numbers to backup the latter point
>  though.

We can try (2), but we will have to benchmark it to determine the
impact on performance.

Thanks, David
David Edelsohn - Nov. 17, 2011, 7:46 p.m.
Coincidentally, IBM's XL compiler is encountering a similar issue and
proposing a similar solution of having the stack pointer update
conflict with all memory accesses.  I think all these corner cases
confirm that it is impractical for the compiler to track all accesses
against SP and the only expedient solution is a hammer like
conflicting with all memory.

Thanks, David
Olivier Hainque - Nov. 18, 2011, 9:05 a.m.
On Nov 17, 2011, at 20:46 , David Edelsohn wrote:

> Coincidentally, IBM's XL compiler is encountering a similar issue and
> proposing a similar solution of having the stack pointer update
> conflict with all memory accesses.

 Interesting timing :-)

> I think all these corner cases
> confirm that it is impractical for the compiler to track all accesses
> against SP and the only expedient solution is a hammer like
> conflicting with all memory.

 OK, I'll come up with a patch. I'll compare the generated assembly for
 the Ada runtime lib with and without the change. That should be easy to
 do and will provide a datapoint.

 Thanks for your feedback.

 Olivier

Patch

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 181085)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11989,7 +11989,7 @@ 
 (define_expand "restore_stack_block"
   [(set (match_dup 2) (match_dup 3))
    (set (match_dup 4) (match_dup 2))
-   (set (match_dup 5) (unspec:BLK [(match_dup 5)] UNSPEC_TIE))
+   (set (mem:BLK (scratch)) (unspec:BLK [(match_dup 5)] UNSPEC_TIE))
    (set (match_operand 0 "register_operand" "")
 	(match_operand 1 "register_operand" ""))]
   ""
@@ -12022,7 +12022,7 @@ 
   [(set (match_dup 2) (match_operand 1 "memory_operand" ""))
    (set (match_dup 3) (match_dup 4))
    (set (match_dup 5) (match_dup 2))
-   (set (match_dup 6) (unspec:BLK [(match_dup 6)] UNSPEC_TIE))
+   (set (mem:BLK (scratch)) (unspec:BLK [(match_dup 6)] UNSPEC_TIE))
    (set (match_operand 0 "register_operand" "") (match_dup 3))]
   ""
   "
@@ -15902,8 +15902,8 @@ 
 ; These are to explain that changes to the stack pointer should
 ; not be moved over stores to stack memory.
 (define_insn "stack_tie"
-  [(set (match_operand:BLK 0 "memory_operand" "+m")
-        (unspec:BLK [(match_dup 0)] UNSPEC_TIE))]
+  [(set (mem:BLK (scratch))
+        (unspec:BLK [(match_operand:BLK 0 "memory_operand" "+m")] UNSPEC_TIE))]
   ""
   ""
   [(set_attr "length" "0")])