[Bug rtl-optimization/47614] New: cpu2000 benchmark 301.apsi fails with revision 169782

[Bug rtl-optimization/47614] New: cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

           Summary: cpu2000 benchmark 301.apsi fails with revision 169782
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: pthaugen@gcc.gnu.org
                CC: aoliva@gcc.gnu.org
              Host: powerpc64-linux
            Target: powerpc64-linux
             Build: powerpc64-linux


Created attachment 23248
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23248
testcase

[Bug rtl-optimization/47614] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

Pat Haugen <pthaugen at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bergner at gcc dot gnu.org,
                   |                            |meissner at gcc dot
                   |                            |gnu.org, wschmidt at gcc
                   |                            |dot gnu.org

--- Comment #1 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-04 22:47:57 UTC ---
Premature bug creation before...

301.apsi started failing on PowerPC with r169782. Attatched testcase was
compiled with 'gfortran -S -m64 -mcpu=power7 -O3 -funroll-loops -ffast-math'.
Following is good/bad snippet of the assembler.

r169781:

        xsmaxdp 0,0,11
        ble 7,.L2
        mr 10,5
        addi 0,7,-2
        lfdu 5,8(10)
        rlwinm 8,0,0,29,31
        li 9,1
        xscmpudp 0,0,5
        ble 0,.L2


r169782:
        xsmaxdp 0,0,11
        ble 7,.L2
        xscmpudp 0,0,11
        addi 0,7,-2
        mr 10,5
        rlwinm 8,0,0,29,31
        li 9,1
        ble 0,.L2

Note in the new version that the lfdu instruction is gone. This doesn't affect
the compare since the memory value in f11 is still valid, the problem is that
elminating the lfdu instruction elminates the increment of r10 which causes
problems later on when it is used in subsequent loads (and is pointing at the
wrong location).

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[rguenth at gcc dot gnu.org]

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

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P1
   Target Milestone|---                         |4.6.0
            Summary|cpu2000 benchmark 301.apsi  |[4.6 Regression] cpu2000
                   |fails with revision 169782  |benchmark 301.apsi fails
                   |                            |with revision 169782

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-02-07 11:54:13 UTC ---
Btw, not really a patch that was appropriate for this stage ...

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-02-07 12:35:06 UTC ---
> Btw, not really a patch that was appropriate for this stage ...

Are you the same Richard Guenther who wrote comment #29 under PR 43494? :-)
That being said, feel free to overrule and declare this 4.7 material.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-02-07 12:49:56 UTC ---
(In reply to comment #3)
> > Btw, not really a patch that was appropriate for this stage ...
> 
> Are you the same Richard Guenther who wrote comment #29 under PR 43494? :-)
> That being said, feel free to overrule and declare this 4.7 material.

"I suppose pinging it on the ML is better than pinging it here."

is a general comment.  I didn't even look at the patch when saying that ;)

A brief look suggests the patch may only change var-tracking behavior, but
it seems that is not the case.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-02-07 18:21:23 UTC ---
> A brief look suggests the patch may only change var-tracking behavior, but
> it seems that is not the case.

Quite very brief then. :-)  PR 43494 was a wrong code regression and
Alexandre's patch fixed it, that's why I looked at it in the first place after
seeing the double ping.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-07 19:13:28 UTC ---
It doesn't affect var-tracking at all BTW, because var-tracking deals with
pre/post_inc/dec/modify manually by changing the insns passed to cselib
temporarily.  It was originally written with the intent to be used in
var-tracking though.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

Pat Haugen <pthaugen at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23248|0                           |1
        is obsolete|                            |

--- Comment #7 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-07 21:18:35 UTC ---
Created attachment 23270
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23270
smaller testcase

A slightly smaller testcase.

Looking at the dumps, postreload is deleting the following insn in r169782.

(insn 95 392 89 3 (set (reg:DF 45 13 [orig:191 MEM[(real(kind=8)[0:]
*)D.1405_52] ] [191])
        (mem:DF (pre_inc:DI (reg:DI 10 10 [orig:216 ivtmp.29 ] [216])) [2
MEM[(real(kind=8)[0:] *)D.1405_52]+0 S8 A64])) fail.f:9 392
{*movdf_hardfloat64}
     (expr_list:REG_INC (reg:DI 10 10 [orig:216 ivtmp.29 ] [216])
        (nil)))

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-08 14:52:25 UTC ---
I can't reproduce this with a cross compiler unfortunately, but perhaps using
check_for_inc_dec (currently static in dse.c) when deleting insn where needed
in postreload.c might fix that.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

--- Comment #9 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-08 21:08:28 UTC ---
Thanks Jakub, check_for_inc_dec() does indeed fix the issue as desired (still
elminates the redundant load, but keeps the increment). I'll fire off
bootstrap/regtest.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-09 18:30:29 UTC ---
Created attachment 23287
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23287
gcc46-pr47614.patch

Actually, I can reproduce it, I've just been looking for pre_modify instead of
pre_inc that is actually removed.

So, either we use check_for_inc_dec there...

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-09 18:31:51 UTC ---
Created attachment 23288
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23288
gcc46-pr47614-2.patch

Or simply reject side effects, similarly how we reject them elsewhere in
postreload.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[ebotcazou at gcc dot gnu.org]

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

--- Comment #12 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-02-09 18:42:01 UTC ---
> Or simply reject side effects, similarly how we reject them elsewhere in
> postreload.

Yes, I think that's good enough for now.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23288|0                           |1
        is obsolete|                            |

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-09 18:48:46 UTC ---
Created attachment 23289
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23289
gcc46-pr47614-2.patch

Actually, the second patch is not enough, we don't delete the insn in that
case, but still reload_cse_simplify_operands optimizes the MEM into a register,
ignoring its side-effects.
I think this should work.  Alternatively, if we go the first patch route, I
think side_effects_p guard in reload_cse_simplify_operands is still desirable.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

--- Comment #14 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-09 21:03:31 UTC ---
(In reply to comment #13)
> Alternatively, if we go the first patch route, I
> think side_effects_p guard in reload_cse_simplify_operands is still desirable.

My bootstrap/regtest (of essentially the first patch above) went fine, but I
agree that the addition of side_effects_p guard in reload_cse_simplify_operands
is probably the right thing to do too.  I like the first patch over the second
since we are able to replace the redundant load with a simple increment.

I'll bootstrap/regtest the additional change.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[jakub at gcc dot gnu.org]

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

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-09 21:10:23 UTC ---
Please move the side_effects_p check in reload_cse_simplify_operands right
before the cselib_lookup call, there is no point calling cselib_lookup if it
has side effects.
BTW, on powerpc64-linux Alex's incdec patch apparently broke profiledbootstrap,
cc1 built with -fprofile-use keeps segfaulting on most of libgcc compilations.

A patch similar to #c13 (just with if (side_effects_p (op)) continue; moved
in front of v = cselib_lookup (...);) fixed the profiledbootstrap apparently
(but guess a patch using check_for_inc_dec + the *_operands change will do the
same).  I agree that in this case it can generate better code to delete the
redundant insn and just keep the side effects.

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

--- Comment #16 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-11 20:52:58 UTC ---
Author: pthaugen
Date: Fri Feb 11 20:52:55 2011
New Revision: 170059

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170059
Log:
    PR rtl-optimization/47614
    * rtl.h (check_for_inc_dec): Declare.
    * dse.c (check_for_inc_dec): Externalize...
    * postreload.c (reload_cse_simplify): ...use it before deleting stmt.
    (reload_cse_simplify_operands): Don't simplify opnds with side effects.

    * testsuite/gfortran.dg/pr47614.f: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/pr47614.f
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dse.c
    trunk/gcc/postreload.c
    trunk/gcc/rtl.h
    trunk/gcc/testsuite/ChangeLog

[Bug rtl-optimization/47614] [4.6 Regression] cpu2000 benchmark 301.apsi fails with revision 169782

[pthaugen at gcc dot gnu.org]

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

Pat Haugen <pthaugen at gcc dot gnu.org> changed:

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

--- Comment #17 from Pat Haugen <pthaugen at gcc dot gnu.org> 2011-02-11 22:01:24 UTC ---
Fixed.