[Bug target/32406] New: [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[Bug target/32406] New: [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

Build from svn r125825 with:
http://gcc.gnu.org/viewcvs?view=rev&revision=125852
applied.

Configured: ../trunk/configure --target=mipsel-linux
--with-sysroot=/usr/local/mipsel-linux-test
--prefix=/usr/local/mipsel-linux-test --with-arch=mips32 --with-float=soft
--disable-java-awt --without-x --disable-tls --enable-__cxa_atexit
--disable-jvmpi --disable-libmudflap --enable-languages=c,c++

gcc.c-torture/execute/nestfunc-6.c is FAILing at -O2, -O3, and -Os.  This is a
regression from:

http://gcc.gnu.org/ml/gcc-testresults/2007-06/msg00546.html

This is the test case:
-----------------------
typedef __SIZE_TYPE__ size_t;
extern void abort (void);
extern void exit (int);
extern void qsort(void *, size_t, size_t, int (*)(const void *, const void *));

int main ()
{
  __label__ nonlocal;
  int compare (const void *a, const void *b)
  {
    goto nonlocal;
  }

  char array[3];
  qsort (array, 3, 1, compare);
  abort ();

 nonlocal:
  exit (0);
}
------------------------------------------------

Here is the generated assembly:
 /home/daney/gccsvn/mipsel-trunk/gcc/xgcc
-B/home/daney/gccsvn/mipsel-trunk/gcc/ -O3 -S nestfunc-6.c
-------------------------------
        .file   1 "nestfunc-6.c"
        .section .mdebug.abi32
        .previous
        .abicalls
        .rdata
        .align  2
$LTRAMP0:
        .word   0x03e00821              # move   $1,$31
        .word   0x04110001              # bgezal $0,.+8
        .word   0x00000000              # nop
        .word   0x8fe30014              # lw     $3,20($31)
        .word   0x8fe20018              # lw     $2,24($31)
        .word   0x0060c821              # move   $25,$3
        .word   0x00600008              # jr     $3
        .word   0x0020f821              # move   $31,$1
        .word   0x00000000              # <function address>
        .word   0x00000000              # <static chain value>
        .globl  _flush_cache
        .text
        .align  2
        .globl  main
        .ent    main
        .type   main, @function
main:
        .frame  $sp,96,$31              # vars= 64, regs= 2/0, args= 16, gp= 8
        .mask   0xc0000000,-4
        .fmask  0x00000000,0
        .set    noreorder
        .set    nomacro

        lui     $28,%hi(__gnu_local_gp)
        addiu   $sp,$sp,-96
        addiu   $28,$28,%lo(__gnu_local_gp)
        sw      $31,92($sp)
        sw      $fp,88($sp)
        .cprestore      16
        lui     $2,%hi($LTRAMP0)
        addiu   $6,$2,%lo($LTRAMP0)
        addiu   $3,$sp,40
        addiu   $2,$sp,24
        sw      $2,84($sp)
        sw      $3,80($sp)
        sw      $2,28($sp)
        sw      $sp,32($sp)
        move    $7,$3
        addiu   $8,$6,32
$L2:
        lw      $2,0($6)
        lw      $3,4($6)
        lw      $4,8($6)
        lw      $5,12($6)
        addiu   $6,$6,16
        sw      $2,0($7)
        sw      $3,4($7)
        sw      $4,8($7)
        sw      $5,12($7)
        bne     $6,$8,$L2
        addiu   $7,$7,16

        lw      $2,4($6)
        lw      $4,0($6)
        sw      $2,4($7)
        sw      $4,0($7)
        lw      $2,80($sp)
        lui     $3,%hi(compare.1584)
        addiu   $3,$3,%lo(compare.1584)
        sw      $3,32($2)
        lw      $3,80($sp)
        addiu   $2,$sp,28
        sw      $2,36($3)
        lw      $25,%call16(_flush_cache)($28)
        lw      $4,80($sp)
        li      $5,40                   # 0x28
        jalr    $25
        li      $6,3                    # 0x3

        lw      $28,16($sp)
        lw      $4,84($sp)
        lw      $25,%call16(qsort)($28)
        lw      $7,80($sp)
        li      $5,3                    # 0x3
        jalr    $25
        li      $6,1                    # 0x1

        lw      $28,16($sp)
        lw      $25,%call16(abort)($28)
        jalr    $25
        nop

$L3:
$L4:
        lw      $25,%call16(exit)($28)
        jalr    $25
        move    $4,$0

        .set    macro
        .set    reorder
        .end    main
        .align  2
        .ent    compare.1584
        .type   compare.1584, @function
compare.1584:
        .frame  $fp,8,$31               # vars= 0, regs= 1/0, args= 0, gp= 0
        .mask   0x40000000,-8
        .fmask  0x00000000,0
        .set    noreorder
        .set    nomacro

        addiu   $sp,$sp,-8
        sw      $fp,0($sp)
        lw      $fp,0($2)
        lw      $sp,4($2)
        lui     $2,%hi($L3)
        addiu   $2,$2,%lo($L3)
        j       $2
        nop

        .set    macro
        .set    reorder
        .end    compare.1584
        .ident  "GCC: (GNU) 4.3.0 20070618 (experimental)"
------------------------------------------------

Note that $gp ($28) is not restored at $L3 where it is used.  The compare
function does 'goto nonlocal;' which is a jump to $L3, but $gp will have the
wrong value and must be restored.

This worked before the dataflow merge because the compare function would load
$gp (unnecessarily).

I think the fix is to load $gp at $L3.


-- 
           Summary: [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: daney at gcc dot gnu dot org
 GCC build triplet: mipsel-linux-gnu
  GCC host triplet: mipsel-linux-gnu
GCC target triplet: mipsel-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[hp at gcc dot gnu dot org]

------- Comment #1 from hp at gcc dot gnu dot org  2007-06-20 02:53 -------
I saw your comments on IRC at an attempt of using nonlocal_goto_receiver.
You don't want to make a new pattern with that name, just rename
exception_receiver; the obvious one-line patch.  That pattern already has the
necessary restore of $gp (not just a clobber).  With a nonlocal_goto_receiver,
the pattern exception_receiver is not used (grep for yourself!), so it doesn't
hurt or something.


-- 

hp at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hp at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

------- Comment #2 from daney at gcc dot gnu dot org  2007-06-20 07:11 -------
Created an attachment (id=13739)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=13739&action=view)
First patch attempt.

I think this patch fixes this bug.  The test case looks better from my
cross-compiler.  I will bootstrap it to be sure.

I don't really like the patch that much though.  It forces $gp to be loaded in
a nonlocal_goto_receiver, which fixes the bug in cases where $gp is needed.  If
$gp is not needed, it would be nice not to force it to be restored.

In vain I tried to mark $gp as clobbered in hope that it would be magically
restored if needed.  I guess I need a bit more RTL foo.  If there are two
function calls in the nonlocal goto target (two uses of $gp with a clobber
between), the second call has $gp restored.  I think there should be a way to
make the first use of $gp to cause $gp to be restored, but I don't know what it
is.

Thanks to Hans-Peter Nilsson for the pointer.


-- 

daney at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |daney at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[richard at codesourcery dot com]

------- Comment #3 from richard at codesourcery dot com  2007-06-20 12:56 -------
Subject: Re:  [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

"daney at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:
> I don't really like the patch that much though.  It forces $gp to be
> loaded in a nonlocal_goto_receiver, which fixes the bug in cases where
> $gp is needed.  If $gp is not needed, it would be nice not to force it
> to be restored.
>
> In vain I tried to mark $gp as clobbered in hope that it would be
> magically restored if needed.  I guess I need a bit more RTL foo.  If
> there are two function calls in the nonlocal goto target (two uses of
> $gp with a clobber between), the second call has $gp restored.  I
> think there should be a way to make the first use of $gp to cause $gp
> to be restored, but I don't know what it is.

The post-reload splitter converts the unspec_volatile into an ordinary
move.  In theory (TM), if the restore isn't needed, it should get deleted
as dead after that point.

Stepping back, the model here is that anything up to and including the
post-reload splitters can introduce new uses of pic_offset_table_rtx.
(This includes reload, which might need new uses of pic_offset_table_rtx
in order to access the constant pool.  It also includes high/lo_sum
accesses that we previously modelled as being purely symbolic;
this higher-level, register-free form is easier to optimise.)

So we basically consider $gp to be live throughout the function before
the post-reload splitters.  At that point, when using explicit relocs,
every use of $gp becomes explicit, and normal liveness rules apply.
Do you have an example of a function that does need $gp at some point,
but which restores $gp unnecessarily at other points?

I suppose there will be cases where a function with a nonlocal goto
doesn't need $gp at all, but sets $gp up anyway because of the set in
the receiver pattern.  I can think of a way of dealing with that,
but it will mean adding special cases.  I wonder how often it would
trigger.

Richard


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

------- Comment #4 from daney at gcc dot gnu dot org  2007-06-22 04:55 -------
Fixed by this patch:

http://gcc.gnu.org/viewcvs?view=rev&revision=125941

Unfortunately I botched the PR number in the ChangeLog with my first commit, so
the magic patch description did not show up here. 


-- 

daney at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

-- 

daney at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

------- Comment #5 from daney at gcc dot gnu dot org  2007-11-11 06:29 -------
*** Bug 34062 has been marked as a duplicate of this bug. ***


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

------- Comment #6 from daney at gcc dot gnu dot org  2007-11-11 06:32 -------
Well the fix only works for o32.  As noted in PR 34062, n32 is broken.  I would
guess n64 as well.

I will take another try at fixing it.  Perhaps the second attempt will work out
a little better than the first.


-- 

daney at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|FIXED                       |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

-- 

daney at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
  GCC build triplet|mipsel-linux-gnu            |mips*-linux-gnu
   GCC host triplet|mipsel-linux-gnu            |mips*-linux-gnu
 GCC target triplet|mipsel-linux-gnu            |mips*-linux-gnu
   Last reconfirmed|0000-00-00 00:00:00         |2007-11-11 06:34:44
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[daney at gcc dot gnu dot org]

------- Comment #7 from daney at gcc dot gnu dot org  2007-11-11 07:05 -------
Created an attachment (id=14528)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14528&action=view)
Proposed patch second half.

Completely untested patch.  It will probably take me a couple of weeks to test
it fully.  I will start testing on mipsel-linux...

Q: Is the first exception_receiver define_expand redundant (with
"TARGET_CALL_CLOBBERED_GP")?  Does it just defer to the nonlocal_goto_receiver?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[rsandifo at gcc dot gnu dot org]

------- Comment #8 from rsandifo at gcc dot gnu dot org  2007-11-11 10:06 -------
Created an attachment (id=14529)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14529&action=view)
Patch that marks $gp as being live on nonlocal jumps

"daney at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:
> Created an attachment (id=14528)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14528&action=view)
>  --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14528&action=view)
> Proposed patch second half.
>
> Completely untested patch.  It will probably take me a couple of weeks
> to test it fully.  I will start testing on mipsel-linux...

You might have noticed already, but this patch won't work.
mips_restore_gp restores $gp from the .cprestore save slot,
which doesn't exist unless TARGET_CALL_CLOBBERED_GP.

TBH, I'd forgotten when reviewing the original patch how this is
supposed to work.  If function Inner has a nonlocal jump to function
Outer, Inner is supposed to initialise $gp on Outer's behalf.
The original form of Inner's prologue does do this, thanks to:

static unsigned int
mips_global_pointer (void)
{
  [...]
  /* If the function has a nonlocal goto, $gp must hold the correct
     global pointer for the target function.  */
  if (current_function_has_nonlocal_goto)
    return GLOBAL_POINTER_REGNUM;

but at higher optimisation levels, the instructions get deleted as
dead for -mno-shared, because nothing in the function itself needs $gp.

I think this is a dataflow bug.  (Dataflow generally, that is, not the
new df machinery.)  The code that emits the nonlocal goto records that
the target of the jump uses the frame and stack pointers:

static rtx
expand_builtin_nonlocal_goto (tree exp)
{
  ...
      /* USE of hard_frame_pointer_rtx added for consistency;
         not clear if really needed.  */
      emit_insn (gen_rtx_USE (VOIDmode, hard_frame_pointer_rtx));
      emit_insn (gen_rtx_USE (VOIDmode, stack_pointer_rtx));

but it doesn't record that it also uses the GP register.  The target-
independent code does handle the liveness of GP in other situations,
thanks originally to flow and now to the df machinery, so I think this
is simply an oversight.  It won't matter for most targets, since the
nonlocal jump involves a symbolic access, and most targets use the GP
for all symbolic accesses or for none.

Long-term, it would be nice to get rid of these sorts of USEs and
handle them entirely in the df machinery.  However, given that we
don't do that yet, I think the best fix is to emit a use of
pic_offset_table_rtx:

      if ((unsigned) PIC_OFFSET_TABLE_REGNUM != INVALID_REGNUM
          && fixed_regs[PIC_OFFSET_TABLE_REGNUM])
        emit_insn (gen_rtx_USE (VOIDmode, pic_offset_table_rtx));

(the condition being the same as that used in flow and df).

I think we can then revert the original patch, which would produce
an unnecessary restore at the target site.

The attached patch seems to work for me, both for o32 and n32.
I'll run some regression tests.

Richard


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[rsandifo at nildram dot co dot uk]

------- Comment #9 from rsandifo at nildram dot co dot uk  2007-11-11 10:16 -------
Subject: Re:  [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

Er, sorry, s/without nonlocal_goto/with nonlocal_goto/ in the patch's
comment.

Richard


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[mmitchel at gcc dot gnu dot org]

-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[rsandifo at gcc dot gnu dot org]

------- Comment #10 from rsandifo at gcc dot gnu dot org  2007-11-28 19:46 -------
Subject: Bug 32406

Author: rsandifo
Date: Wed Nov 28 19:46:03 2007
New Revision: 130493

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=130493
Log:
gcc/
        PR target/32406
        * builtins.c (expand_builtin_nonlocal_goto): Also emit a use
        of GP register, if valid and fixed.

        Revert:
        2007-06-21  David Daney  <ddaney@avtrex.com>

        PR target/32406
        * config/mips/mips.md (define_constants): Rename UNSPEC_EH_RECEIVER
        to UNSPEC_NONLOCAL_GOTO_RECEIVER globally.
        (exception_receiver): Renamed to ...
        (nonlocal_goto_receiver): ... this.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/builtins.c
    trunk/gcc/config/mips/mips.md


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406

[Bug target/32406] [4.3 Regression] MIPS: FAIL in nestfunc-6.c at -O3

[rsandifo at gcc dot gnu dot org]

------- Comment #11 from rsandifo at gcc dot gnu dot org  2007-11-28 19:49 -------
Patch applied.  Thanks to David for the testing and
Mark for the review.


-- 

rsandifo at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32406