ppc glibc 2.3 failure

ppc glibc 2.3 failure

[Aldy Hernandez]

Hi guys.

The following distilled case from glibc from CVS...

    char *banner;
    extern int __write (char *) __attribute__ ((visibility ("hidden")));
    void foo() { __write (banner); }

...fails to compile with -O2 -fpic on linuxppc:

a.c:4: error: Attempt to delete prologue/epilogue insn:
(insn 32 31 33 0 (nil) (set (reg:SI 65 lr)
        (reg:SI 11 r11)) -1 (nil)
    (nil))

I haven't done *deep* debugging, but the problem goes away if I remove
Dale's patch below.

Here is what happens: at prologue time, we set LR here (because
info_ptr->lr_save_p is false, etc):

   if (save_LR_around_toc_setup)
      emit_move_insn (gen_rtx_REG (Pmode, LINK_REGISTER_REGNUM),
                      gen_rtx_REG (Pmode, 11));

sometime between prologue generation and epilogue generation we call
rs6000_stack_info(), and end up setting info_ptr->lr_save_p to true,
which will cause us to generate this in the epilogue:

  /* Set LR here to try to overlap restores below.  */
  if (info->lr_save_p)
    emit_move_insn (gen_rtx_REG (Pmode, LINK_REGISTER_REGNUM),
                    gen_rtx_REG (Pmode, 0));

So what we have is rs6000_stack_info behaving different in subsequent
runs, and I've traced it to rs6000_ra_ever_killed() giving different
results.  This will have us generating:

	  (set (reg lr) blah)
	  stuff
	  (set (reg lr) blah)

The first set (in the prologue) is dead, and flow will try to delete
it.

Dale, could you take a look at this?

2002-08-09  Dale Johannesen  <dalej@apple.com>

        * config/rs6000/rs6000.md: Add sibcall patterns.
        * config/rs6000/rs6000.h (FUNCTION_OK_FOR_SIBCALL):  Define.
        * config/rs6000/rs6000.c (rs6000_ra_ever_killed):
        Rewritten to handle sibcalls.
        * config/rs6000/rs6000.c (function_ok_for_sibcall):  New.
        * config/rs6000/rs6000-protos.h (function_ok_for_sibcall):  New.

Re: ppc glibc 2.3 failure

[Dale Johannesen]

On Monday, October 7, 2002, at 04:06  PM, Aldy Hernandez wrote:

> Hi guys.
>
> The following distilled case from glibc from CVS...
>
> ...So what we have is rs6000_stack_info behaving different in 
> subsequent
> runs, and I've traced it to rs6000_ra_ever_killed() giving different
> results.

This should not happen.  As the comments in
rs6000_ra_ever_killed indicate, its expectation is that code (in 
particular,
copies into LR) added in the prolog have a REG_MAYBE_DEAD note, to 
prevent
this problem.  I can't test it, but try this:

     if (save_LR_around_toc_setup)
       rs6000_maybe_dead (
       emit_move_insn (gen_rtx_REG (Pmode, LINK_REGISTER_REGNUM),
                       gen_rtx_REG (Pmode, 11)));

Re: ppc glibc 2.3 failure

[Aldy Hernandez]

> This should not happen.  As the comments in
> rs6000_ra_ever_killed indicate, its expectation is that code (in 
> particular,
> copies into LR) added in the prolog have a REG_MAYBE_DEAD note, to 
> prevent
> this problem.  I can't test it, but try this:
> 
>     if (save_LR_around_toc_setup)
>       rs6000_maybe_dead (
>       emit_move_insn (gen_rtx_REG (Pmode, LINK_REGISTER_REGNUM),
>                       gen_rtx_REG (Pmode, 11)));

Yup, that fixes it.

Is this the correct solution?  If so, I can test out the patch
overnight.

Aldy

Re: ppc glibc 2.3 failure

[Dale Johannesen]

On Monday, October 7, 2002, at 04:37  PM, Aldy Hernandez wrote:

>> This should not happen.  As the comments in
>> rs6000_ra_ever_killed indicate, its expectation is that code (in
>> particular,
>> copies into LR) added in the prolog have a REG_MAYBE_DEAD note, to
>> prevent
>> this problem.  I can't test it, but try this:
>>
>>     if (save_LR_around_toc_setup)
>>       rs6000_maybe_dead (
>>       emit_move_insn (gen_rtx_REG (Pmode, LINK_REGISTER_REGNUM),
>>                       gen_rtx_REG (Pmode, 11)));
>
> Yup, that fixes it.
>
> Is this the correct solution?
Yes, I think so.  I ran into the same problem on Darwin and found
the REG_MAYBE_DEAD notes as a way to identify code added in the prolog.
I wouldn't object to a more elegant identification, but I don't know
of anything else that works.

> If so, I can test out the patch overnight.
Thanks.

Re: ppc glibc 2.3 failure

[Richard Henderson]

On Mon, Oct 07, 2002 at 04:51:54PM -0700, Dale Johannesen wrote:
> >Is this the correct solution?
>
> Yes, I think so.  I ran into the same problem on Darwin and found
> the REG_MAYBE_DEAD notes as a way to identify code added in the prolog.

I don't think this is right.

Basically, the prologue has taken care of the link register.
This is clear from the fact that lr_save_p is false.  Thus it is
clear that if the epilogue emits any code to restore lr, that is
a bug.  We should not be hiding this bug by marking the prologue
instruction as maybe_dead.  It _isn't_ dead.  How can the epilogue
restore the link register if it wasn't saved in the prologue?

rs6000_stack_info needs to produce different results when called
during the reload loop (since we might be spilling more and more
registers), but after reload is complete, the values computed
must *never* change.

See ia64_compute_frame_size and how it handles "initialized".



r~

Re: ppc glibc 2.3 failure

[Dale Johannesen]

On Monday, October 7, 2002, at 05:43  PM, Richard Henderson wrote:

> On Mon, Oct 07, 2002 at 04:51:54PM -0700, Dale Johannesen wrote:
>>> Is this the correct solution?
>>
>> Yes, I think so.  I ran into the same problem on Darwin and found
>> the REG_MAYBE_DEAD notes as a way to identify code added in the 
>> prolog.
>
> I don't think this is right.

Well, it's "right" in the sense that it works.  It looks like you want
to add a mechanism so the stack frame computations are remembered rather
than recomputed, when possible.  I agree that's more elegant, not to
mention faster; I'll look at it.  (The current design is not mine, BTW.)

> Basically, the prologue has taken care of the link register.
> This is clear from the fact that lr_save_p is false.  Thus it is
> clear that if the epilogue emits any code to restore lr, that is
> a bug.  We should not be hiding this bug by marking the prologue
> instruction as maybe_dead.  It _isn't_ dead.  How can the epilogue
> restore the link register if it wasn't saved in the prologue?
>
> rs6000_stack_info needs to produce different results when called
> during the reload loop (since we might be spilling more and more
> registers), but after reload is complete, the values computed
> must *never* change.
>
> See ia64_compute_frame_size and how it handles "initialized".

Re: ppc glibc 2.3 failure

[Richard Henderson]

On Mon, Oct 07, 2002 at 06:09:31PM -0700, Dale Johannesen wrote:
> Well, it's "right" in the sense that it works.

It is not "right", however, in the sense that a prologue instruction
gets marked REG_MAYBE_DEAD, when the instruction is not in fact dead.
Thus bugs elsewhere in post-reload optimizers may cause flow2 to
delete the instruction instead of aborting as it ought.

A correct solution that retains the current structure would use
prologue_epilogue_contains instead of REG_MAYBE_DEAD.  Of course,
that's pointless since we can just avoid recomputing this information
in the first place.  :-)


r~

Re: ppc glibc 2.3 failure

[Geoff Keating]

Dale Johannesen <dalej@apple.com> writes:

> On Monday, October 7, 2002, at 04:06  PM, Aldy Hernandez wrote:
> 
> > Hi guys.
> >
> > The following distilled case from glibc from CVS...
> >
> > ...So what we have is rs6000_stack_info behaving different in
> > subsequent
> > runs, and I've traced it to rs6000_ra_ever_killed() giving different
> > results.
> 
> This should not happen.  As the comments in
> rs6000_ra_ever_killed indicate, its expectation is that code (in
> particular,
> copies into LR) added in the prolog have a REG_MAYBE_DEAD note, to
> prevent
> this problem. 

That's not right.  REG_MAYBE_DEAD should only be applied to those
stores in the prologue that might actually become dead, not to every
store or every insn.

The proper way to test if an instruction is in the prologue is to use
the function named "prologue_epilogue_contains".

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: ppc glibc 2.3 failure

[Aldy Hernandez]

On Mon, Oct 07, 2002 at 05:43:36PM -0700, Richard Henderson wrote:

> rs6000_stack_info needs to produce different results when called
> during the reload loop (since we might be spilling more and more
> registers), but after reload is complete, the values computed
> must *never* change.
> 
> See ia64_compute_frame_size and how it handles "initialized".

I tried to do it the ia64 way, but ran into a problem where we set the
LR in the prologue, and then emit the following call in the body:

(call_insn 35 33 37 0 0x30267480 (parallel [
            (set (reg:SI 3 r3)
                (call (mem:SI (symbol_ref:SI ("getpid")) [0 S4 A8])
                    (const_int 0 [0x0])))
            (use (const_int 0 [0x0]))
            (clobber (reg:SI 65 lr))
	    ^^^^^^^^^^^^^^^^ boo hiss!

As you've suggested, I'm looking at why rs6000_stack_info() did not
pick the clobber in the first place.

Meanwhile... I already had this patch tested, changing
rs6000_ra_ever_killed to use prologue_epilogue_contains.  I updated
comments and typos throughout as well.

Tested on ppc linux.  Bootstrapped and tested everything except Java,
since it's broken on ppc linux right now.

Is this ok (while I investigate why caching the stack info is
broken)?  I just have this pet peeve cuz I can't build glibc :).

Aldy

Re: ppc glibc 2.3 failure

[Dale Johannesen]

On Tuesday, October 8, 2002, at 04:48  PM, Aldy Hernandez wrote:

> On Mon, Oct 07, 2002 at 05:43:36PM -0700, Richard Henderson wrote:
>
>> rs6000_stack_info needs to produce different results when called
>> during the reload loop (since we might be spilling more and more
>> registers), but after reload is complete, the values computed
>> must *never* change.
>>
>> See ia64_compute_frame_size and how it handles "initialized".
>
> I tried to do it the ia64 way, but ran into a problem...

> Meanwhile... I already had this patch tested, changing
> rs6000_ra_ever_killed to use prologue_epilogue_contains.  I updated
> comments and typos throughout as well.

I have no authority, but this is generally OK with me (be nice to see 
the patch).
Too bad nobody noticed this when I first submitted the sibcall patch, 
but that's
how it goes.