[Bug middle-end/33638] New: optimization bug: wrong code with -fforce-addr

[Bug middle-end/33638] New: optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

I have a rather nasty optimization issue with gfortran (as of yesterday). 
As I think it could be an optimization issue and not an gfortran frontend
issue, so I assigned it to the middle-end.

I narrowed my problem to one single fortran function. If I compile this
function with
"gfortran -O2 -march=pentium4" the output is OK, using
"gfortran -O2 -march=pentium4 -fforce-addr" produces wrong output.

When reducing optimization level to "-O1", the issue vanishes for all tried
flag combinations.
If I comment out a particular non-functional line in this code, the issue goes
away:

if (always_true) then
  ....
else
   this_line_commented_out_makes_it_working_again
   ....
endif

I will add assembler output of original, non-working code as well as
of working code when commenting the non-functional line, both compiled with
"gfortran -O2 -march=pentium4 -fforce-addr -S comunpack.f"
Function code is added as well.


Further data points:
- output of -fdump-tree-optimized is the same for both the working and the 
    broken case (of the original function, not the line commenting test).
- there are differences in assembler output though.
- the code calls C functions.
- I could reproduce this issue on an different, non-pentium4 machine 
    using the same flags.
- the flag -march=pentium4 is necessary to trigger the issue.


-- 
           Summary: optimization bug: wrong code with -fforce-addr
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: manfred99 at gmx dot ch


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #1 from manfred99 at gmx dot ch  2007-10-03 12:46 -------
Created an attachment (id=14288)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14288&action=view)
Source code of affected function


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #2 from manfred99 at gmx dot ch  2007-10-03 12:47 -------
Created an attachment (id=14289)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14289&action=view)
Assembler code of original code


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #3 from manfred99 at gmx dot ch  2007-10-03 12:49 -------
Created an attachment (id=14290)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14290&action=view)
Assembler code when commenting line 315, works


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #4 from ubizjak at gmail dot com  2007-10-03 15:43 -------
Please provide enough sources to create an _executable_ that shows the failure.
We are dealing with runtime failure here.

A _short_ testcase (<30 lines) is nice to have, so all non-related parts should
be removed. Wandering through a maze of jumps certainly doesn't help to find
the problem.

An abort() should be called for wrong result, but at least wrong result and
expected result should be described to help trace the issue down.

BTW: Assembler dumps are not _that_ informative, especially without
-fverbose-asm ;)


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #5 from manfred99 at gmx dot ch  2007-10-03 20:10 -------
Subject: Re:  optimization bug: wrong code with
 -fforce-addr


> ------- Comment #4 from ubizjak at gmail dot com  2007-10-03 15:43 -------
> Please provide enough sources to create an _executable_ that shows the
> failure.
> We are dealing with runtime failure here.
>

If I could come up with a short testcase just like that, 
I would have done so already.

> A _short_ testcase (<30 lines) is nice to have, so all non-related parts
> should
> be removed. Wandering through a maze of jumps certainly doesn't help to
> find
> the problem.
> 
> An abort() should be called for wrong result, but at least wrong result
> and
> expected result should be described to help trace the issue down.
> 

I will see what I can do, but do not expect any miracles.
This is from a large software package with ~10^2 functions
and at the end there is a binary output file of several MB which is
either as it should or not :-(

And as I told in the bug report, inserting or deleting code suddenly
let the issue vanish. I already tried to reduce this code with the 
result that removing dead code fixed the miscompilation.

> BTW: Assembler dumps are not _that_ informative, especially without
> -fverbose-asm ;)
> 

As it was not clear to me what information to provide, I asked first
on the gcc mailing list, to no avail, so I provided just some random
information.


BTW 1:
# gfortran -v
Using built-in specs.
Target: i386-pc-linux-gnu
Configured with: /home/fx/gfortran_nightbuild/trunk/configure
--prefix=/home/fx/gfortran_nightbuild/irun-20071001
--enable-languages=c,fortran --build=i386-pc-linux-gnu
--enable-checking=release --with-gmp=/home/fx/gfortran_nightbuild/software
Thread model: posix
gcc version 4.3.0 20071001 (experimental) [trunk revision 128899] (GCC)


BTW 2:
Code was checked with valgrind and efence, seems to be ok.


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #6 from ubizjak at gmail dot com  2007-10-04 06:48 -------
> And as I told in the bug report, inserting or deleting code suddenly
> let the issue vanish. I already tried to reduce this code with the 
> result that removing dead code fixed the miscompilation.

If this is the case, then providing missing functions would be OK too. They can
be dummy functions.

gfortran -O2 -march=pentium4 -fforce-addr pr33638.f
/tmp/cctJqmAN.o: In function `comunpack_':
pr33638.f:(.text+0x47): undefined reference to `rdieee_'
pr33638.f:(.text+0x3c9): undefined reference to `gbytes_'
pr33638.f:(.text+0x456): undefined reference to `gbytes_'
pr33638.f:(.text+0x4df): undefined reference to `gbytes_'
pr33638.f:(.text+0x6fc): undefined reference to `gbytes_'
pr33638.f:(.text+0x778): undefined reference to `gbyte_'
pr33638.f:(.text+0x7aa): undefined reference to `gbyte_'
pr33638.f:(.text+0x7f5): undefined reference to `gbyte_'
pr33638.f:(.text+0x824): undefined reference to `gbyte_'
pr33638.f:(.text+0x85d): undefined reference to `rdieee_'
pr33638.f:(.text+0x8dc): undefined reference to `rdieee_'
pr33638.f:(.text+0x8f9): undefined reference to `rdieee_'
pr33638.f:(.text+0xa3f): undefined reference to `gbytes_'
pr33638.f:(.text+0xc1a): undefined reference to `gbyte_'
pr33638.f:(.text+0xc49): undefined reference to `gbyte_'
/usr/local.uros/lib/gcc/i686-pc-linux-gnu/4.3.0/libgfortranbegin.a(fmain.o): In
function `main':
/home/uros/gcc-build/i686-pc-linux-gnu/libgfortran/../../../gcc-svn/trunk/libgfortran/fmain.c:21:
undefined reference to `MAIN__'
collect2: ld returned 1 exit status

> I will see what I can do, but do not expect any miracles.
> This is from a large software package with ~10^2 functions
> and at the end there is a binary output file of several MB which is
> either as it should or not :-(

If you can step through the executable testcase with gdb and if you can say
that "fld(5) value at line XXX is YYY but should be WWW" is enough to find the
failure.

> As it was not clear to me what information to provide, I asked first
> on the gcc mailing list, to no avail, so I provided just some random
> information.

Well, we have bugzilla to report and manage bugs.


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #7 from manfred99 at gmx dot ch  2007-10-05 09:36 -------
Ok, I managed to produce a testcase: a wrapper around the miscompiled
function and all the missing routines. Input data is read from a binary file,
so the program has to be run on a x86 machine (littleendian).

I attach a tar file with all these things, including program output of a
good and a bad run.

good.out is an output of
# gcc -O2 -march=pentium4 -c mova2i.c
# gfortran -O2 -march=pentium4 -o main main.f \
    comunpack.f rdieee.f gbytesc.f mova2i.o
# ./main > good.out

bad.out likewise, but with "-O2 -march=pentium4 -fforce-addr" (the
additional flag only matters to comunpack.f, the other routines seem
not to be affected and can be compiled arbitrarily).

Hope this helps.


-- 


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

[Bug middle-end/33638] optimization bug: wrong code with -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #8 from manfred99 at gmx dot ch  2007-10-05 09:39 -------
Created an attachment (id=14300)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14300&action=view)
test case


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #9 from ubizjak at gmail dot com  2007-10-05 10:47 -------
> Hope this helps.
Sure, I've got the problem. The problem is actually in RTL optimization, where
dse1 pass removes wrong insn.

Suprisingly, the problem is in line 61 of comunpack.f:

-->   bscale = 2.0**real(idrstmpl(2))
      dscale = 10.0**real(-idrstmpl(3))

This expands into:

=====8<=====
;; bscale = __builtin_powf (2.0e+0, (real4) D.1062)
(insn 1310 1309 1311 pr33638.f:61 (set (reg:SF 646)
        (float:SF (reg:SI 182 [ D.1062 ]))) -1 (nil))

(insn 1311 1310 1312 pr33638.f:61 (parallel [
            (set (reg/f:SI 647)
                (plus:SI (reg/f:SI 56 virtual-outgoing-args)
                    (const_int 4 [0x4])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

(insn 1312 1311 1313 pr33638.f:61 (set (mem:SF (reg/f:SI 647) [0 S4 A32])
        (reg:SF 646)) -1 (nil))

(insn 1313 1312 1314 pr33638.f:61 (set (reg:SF 648)
        (mem/u/c/i:SF (symbol_ref/u:SI ("*.LC4") [flags 0x2]) [9 S4 A32])) -1
(nil))

(insn 1314 1313 1315 pr33638.f:61 (set (mem:SF (reg/f:SI 56
virtual-outgoing-args) [0 S4 A32])
        (reg:SF 648)) -1 (nil))

(call_insn/u 1315 1314 1316 pr33638.f:61 (set (reg:SF 8 st)
        (call (mem:QI (symbol_ref:SI ("powf") [flags 0x41] <function_decl
0xb7c1c200 __builtin_powf>) [0 S1 A8])
            (const_int 8 [0x8]))) -1 (nil)
    (expr_list:REG_DEP_TRUE (use (mem/i:SF (reg/f:SI 56 virtual-outgoing-args)
[0 S4 A32]))
        (expr_list:REG_DEP_TRUE (use (mem/i:SF (plus:SI (reg/f:SI 56
virtual-outgoing-args)
                        (const_int 4 [0x4])) [0 S4 A32]))
            (nil))))
=====>8=====

and in the second powf call, we have similar sequence with (insn 1321) and
(insn 1322):

=====8<=====
(insn 1321 1320 1322 pr33638.f:62 (parallel [
            (set (reg/f:SI 653)
                (plus:SI (reg/f:SI 56 virtual-outgoing-args)
                    (const_int 4 [0x4])))
            (clobber (reg:CC 17 flags))
        ]) -1 (nil))

(insn 1322 1321 1323 pr33638.f:62 (set (mem:SF (reg/f:SI 653) [0 S4 A32])
        (reg:SF 652)) -1 (nil))
=====>8=====

Important instructions are (insn 1311) - calculation of ebp+4, (insn 1312) -
store to calculated address and equivalent (insn 1321) and (insn 1322). First,
gcse1 pass correctly eliminates (insn 1311) and (insn 1321), so we got:

=====8<=====
(insn 1310 1309 1312 150 pr33638.f:61 (set (reg:SF 646)
        (float:SF (reg:SI 182 [ D.1062 ]))) 148 {*floatsisf2_i387}
(expr_list:REG_DEAD (reg:SI 182 [ D.1062 ])
        (nil)))

(insn 1312 1310 1313 150 pr33638.f:61 (set (mem:SF (reg/f:SI 755) [0 S4 A32])
        (reg:SF 646)) 66 {*movsf_1} (expr_list:REG_DEAD (reg:SF 646)
        (nil)))

(insn 1313 1312 1314 150 pr33638.f:61 (set (reg:SF 648)
        (mem/u/c/i:SF (symbol_ref/u:SI ("*.LC4") [flags 0x2]) [9 S4 A32])) 66
{*movsf_1} (expr_list:REG_EQUAL (const_double:SF -2147483648 [0x80000000]
2.0e+0 [0x0.8p+2])
        (nil)))

(insn 1314 1313 1315 150 pr33638.f:61 (set (mem:SF (reg/f:SI 7 sp) [0 S4 A32])
        (reg:SF 648)) 66 {*movsf_1} (expr_list:REG_DEAD (reg:SF 648)
        (nil)))

(call_insn/u 1315 1314 1316 150 pr33638.f:61 (set (reg:SF 8 st)
        (call (mem:QI (symbol_ref:SI ("powf") [flags 0x41] <function_decl
0xb7c1c200 __builtin_powf>) [0 S1 A8])
            (const_int 8 [0x8]))) 611 {*call_value_0} (nil)
    (expr_list:REG_DEP_TRUE (use (mem/i:SF (reg/f:SI 7 sp) [0 S4 A32]))
        (expr_list:REG_DEP_TRUE (use (mem/i:SF (plus:SI (reg/f:SI 7 sp)
                        (const_int 4 [0x4])) [0 S4 A32]))
            (nil))))

...

(insn 1322 1320 1323 150 pr33638.f:62 (set (mem:SF (reg/f:SI 755) [0 S4 A32])
        (reg:SF 652)) 66 {*movsf_1} (expr_list:REG_DEAD (reg/f:SI 755)
        (expr_list:REG_DEAD (reg:SF 652)
            (nil))))
=====>8=====

where (reg 755) in (insn 1312) is correctly marked as live-in register:

;; live  in      6 [bp] 7 [sp] 16 [argp] 20 [frame] 137 138 139 140 142 181 182
188 189 749 755
;; live  gen     8 [st] 17 [flags] 646 648 649 651 652 654 655
;; live  kill    17 [flags]


We proceed to dse1 pass where (insn 1312):

**scanning insn=1312
  mem: (reg/f:SI 755)
expanding: r755 into: NULL

   after cselib_expand address: (reg/f:SI 755)

   after canon_rtx address: (reg/f:SI 755)
  varying cselib base=2 offset = 0
 processing cselib store [0..4)
mems_found = 1, cannot_delete = false


and (insn 1322):

**scanning insn=1322
  mem: (reg/f:SI 755)
expanding: r755 into: NULL

   after cselib_expand address: (reg/f:SI 755)

   after canon_rtx address: (reg/f:SI 755)
  varying cselib base=2 offset = 0
 processing cselib store [0..4)
    trying store in insn=1312 gid=-1[0..4)
Locally deleting insn 1312 
deferring deletion of insn with uid = 1312.
mems_found = 1, cannot_delete = false

This is wrong. dse1 pass should _not_ remove (insn 1312). (insn 1322) is
storing to the same address as (insn 1312), but we have a call to powf in
between, so new store insn doesn't kill previous store.

Confirmed as a regression on i686-pc-linux-gnu on comunpack.f using '-O2
-march=pentium4 -fforce-addr'


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|middle-end                  |rtl-optimization
     Ever Confirmed|0                           |1
 GCC target triplet|                            |i686-pc-linux-gnu
   Last reconfirmed|0000-00-00 00:00:00         |2007-10-05 10:47:20
               date|                            |
            Summary|optimization bug: wrong code|[4.3 regression]: wrong code
                   |with -fforce-addr           |with -fforce-addr


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #10 from ubizjak at gmail dot com  2007-10-05 11:40 -------
(In reply to comment #9)

> This is wrong. dse1 pass should _not_ remove (insn 1312). (insn 1322) is
> storing to the same address as (insn 1312), but we have a call to powf in
> between, so new store insn doesn't kill previous store.

CCing authors of RTL DSE pass.


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zadeck at naturalbridge dot
                   |                            |com, rsandifo at gcc dot gnu
                   |                            |dot org


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

-- 

zadeck at naturalbridge dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |zadeck at naturalbridge dot
                   |dot org                     |com
             Status|NEW                         |ASSIGNED


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[rguenth at gcc dot gnu dot org]

------- Comment #11 from rguenth at gcc dot gnu dot org  2007-10-05 12:36 -------
But powf is pure/const, so the call is not a use.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

------- Comment #12 from zadeck at naturalbridge dot com  2007-10-05 13:02 -------
Subject: Re:  [4.3 regression]: wrong code with
 -fforce-addr

rguenth at gcc dot gnu dot org wrote:
> ------- Comment #11 from rguenth at gcc dot gnu dot org  2007-10-05 12:36 -------
> But powf is pure/const, so the call is not a use.
>
>
>   
that is the reason that the call did not kill the potential set of dead
stores.

I will look at this later today.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[kargl at gcc dot gnu dot org]

------- Comment #13 from kargl at gcc dot gnu dot org  2007-10-05 17:50 -------
(In reply to comment #9)
> > Hope this helps.
> Sure, I've got the problem. The problem is actually in RTL optimization, where
> dse1 pass removes wrong insn.
> 
> Suprisingly, the problem is in line 61 of comunpack.f:
> 
> -->   bscale = 2.0**real(idrstmpl(2))
>       dscale = 10.0**real(-idrstmpl(3))
> 

This meant for Manfred instead of Uros, but it does contain the 
relevant info.  Manfred, you told me elsewhere that you use -fforce-addr
to achieve better performance.  Whoever wrote this code should be
flogged.  idrstmpl is an INTEGER variable, and gfortran can generate
much faster code for integer exponents than calling __builtin_powf.

Try changing the lines to

  bscale = 2.0**idrstmpl(2)
  dscale = 10.0**(-idrstmpl(3))

This, of course, doesn't fix the underlying bug.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[rguenth at gcc dot gnu dot org]

------- Comment #14 from rguenth at gcc dot gnu dot org  2007-10-05 20:16 -------
The testcase works for me, that is, it produces the expected output good.out.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

------- Comment #15 from zadeck at naturalbridge dot com  2007-10-05 20:17 -------
Subject: Re:  [4.3 regression]: wrong code with
 -fforce-addr

kargl at gcc dot gnu dot org wrote:
> ------- Comment #13 from kargl at gcc dot gnu dot org  2007-10-05 17:50 -------
> (In reply to comment #9)
>   
>>> Hope this helps.
>>>       
>> Sure, I've got the problem. The problem is actually in RTL optimization, where
>> dse1 pass removes wrong insn.
>>
>> Suprisingly, the problem is in line 61 of comunpack.f:
>>
>> -->   bscale = 2.0**real(idrstmpl(2))
>>       dscale = 10.0**real(-idrstmpl(3))
>>
>>     
>
> This meant for Manfred instead of Uros, but it does contain the 
> relevant info.  Manfred, you told me elsewhere that you use -fforce-addr
> to achieve better performance.  Whoever wrote this code should be
> flogged.  idrstmpl is an INTEGER variable, and gfortran can generate
> much faster code for integer exponents than calling __builtin_powf.
>
> Try changing the lines to
>
>   bscale = 2.0**idrstmpl(2)
>   dscale = 10.0**(-idrstmpl(3))
>
> This, of course, doesn't fix the underlying bug.
>
>
>   
neither richi nor myself are able to reproduced the problem. 

./xgcc -B. -O2 -march=pentium4 -c mova2i.c -DLINUX
./gfortran -fforce-addr -B. -B../i686-pc-linux-gnu/libgfortran/.libs  -O2 -
march=pentium4 -o main main.f     comunpack.f rdieee.f gbytesc.f mova2i.o

and i get the same thing with and without -fforce-addr


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #16 from ubizjak at gmail dot com  2007-10-06 06:49 -------
(In reply to comment #14)
> The testcase works for me, that is, it produces the expected output good.out.

Uh, you have to un-comment the line 315 of the comunpack.f test. The testcase,
as attached, produces good code. Un-commenting line 315, you will get:

.L80:
        movl    $0x40000000, (%esp)
        call    powf
        fstps   -152(%ebp)
        negl    -136(%ebp)
        fildl   -136(%ebp)
        fstps   4(%esp)
        movl    $0x41200000, (%esp)
        call    powf

Note that only one argument is loaded to the stack before first powf.

Without -fforce-address on un-commented testcase, we got:

.L80:
        fildl   -132(%ebp)
        fstps   4(%esp)
        movl    $0x40000000, (%esp)
        call    powf
        fstps   -140(%ebp)
        negl    -128(%ebp)
        fildl   -128(%ebp)
        fstps   4(%esp)
        movl    $0x41200000, (%esp)
        call    powf


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

------- Comment #17 from zadeck at naturalbridge dot com  2007-10-06 12:27 -------
Subject: Re:  [4.3 regression]: wrong code with
 -fforce-addr

ubizjak at gmail dot com wrote:
> ------- Comment #16 from ubizjak at gmail dot com  2007-10-06 06:49 -------
> (In reply to comment #14)
>   
>> The testcase works for me, that is, it produces the expected output good.out.
>>     
>
> Uh, you have to un-comment the line 315 of the comunpack.f test. The testcase,
> as attached, produces good code. Un-commenting line 315, you will get:
>
>
>   
you are making this into something of a scavenger hunt.

Kenny


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

------- Comment #18 from zadeck at naturalbridge dot com  2007-10-06 13:07 -------
Subject: Re:  [4.3 regression]: wrong code with
 -fforce-addr

ubizjak at gmail dot com wrote:
> ------- Comment #16 from ubizjak at gmail dot com  2007-10-06 06:49 -------
> (In reply to comment #14)
>   
>> The testcase works for me, that is, it produces the expected output good.out.
>>     
>
> Uh, you have to un-comment the line 315 of the comunpack.f test. The testcase,
> as attached, produces good code. Un-commenting line 315, you will get:
>
> .L80:
>         movl    $0x40000000, (%esp)
>         call    powf
>         fstps   -152(%ebp)
>         negl    -136(%ebp)
>         fildl   -136(%ebp)
>         fstps   4(%esp)
>         movl    $0x41200000, (%esp)
>         call    powf
>
> Note that only one argument is loaded to the stack before first powf.
>
> Without -fforce-address on un-commented testcase, we got:
>
> .L80:
>         fildl   -132(%ebp)
>         fstps   4(%esp)
>         movl    $0x40000000, (%esp)
>         call    powf
>         fstps   -140(%ebp)
>         negl    -128(%ebp)
>         fildl   -128(%ebp)
>         fstps   4(%esp)
>         movl    $0x41200000, (%esp)
>         call    powf
>
>
>   
ian,

As you may remember, the dse code assumes that it can "see" all of the
stores that are frame_related.   It appears that with the  -fforce-addr
option this is not true.  in this particular example, a frame related
pointer gets loaded into register 755 very early on (in a different
block) and since const calls only disqualify frame-related stores,
(since they may push params onto the stack), the parameter push is
considered dead.

My question to you, is the proper fix to check flag_force-addr and if it
is set just assume that every store may be frame related or is there
some sort of tea leaf that i might have access to know that reg 755 is
used in this way?

(note that you have to jump thru a few hoops to recreate this, since
comunpack.f is in a separate attachment from the rest of the code and
you have to uncomment line 315 to recreate the bug.)


Kenny


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #19 from ubizjak at gmail dot com  2007-10-06 19:58 -------
In dse.c, scan_insn(), we have:

  if ((GET_CODE (PATTERN (insn)) == CLOBBER)
      || volatile_refs_p (PATTERN (insn))
      || (flag_non_call_exceptions && may_trap_p (PATTERN (insn)))
      || (RTX_FRAME_RELATED_P (insn))
      || find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX))
    insn_info->cannot_delete = true;

And since the docs say that:

`RTX_FRAME_RELATED_P (X)'
     Nonzero in an `insn', `call_insn', `jump_insn', `barrier', or
     `set' which is part of a function prologue and sets the stack
     pointer, sets the frame pointer, or saves a register.  This flag
     should also be set on an instruction that sets up a temporary
     register to use in place of the frame pointer.  Stored in the
     `frame_related' field and printed as `/f'.

I wonder if the insn that stores to (or uses(?)) this temporary register (in
place of the frame pointer) should also be marked as frame related insn?

So, all the insns in the sequence of

set tmpreg, FP + const
...
store (tmpreg)

should be marked as frame related insns.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[zadeck at naturalbridge dot com]

------- Comment #20 from zadeck at naturalbridge dot com  2007-10-06 21:20 -------
Subject: Re:  [4.3 regression]: wrong code with
 -fforce-addr

ubizjak at gmail dot com wrote:
> ------- Comment #19 from ubizjak at gmail dot com  2007-10-06 19:58 -------
> In dse.c, scan_insn(), we have:
>
>   if ((GET_CODE (PATTERN (insn)) == CLOBBER)
>       || volatile_refs_p (PATTERN (insn))
>       || (flag_non_call_exceptions && may_trap_p (PATTERN (insn)))
>       || (RTX_FRAME_RELATED_P (insn))
>       || find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX))
>     insn_info->cannot_delete = true;
>
> And since the docs say that:
>
> `RTX_FRAME_RELATED_P (X)'
>      Nonzero in an `insn', `call_insn', `jump_insn', `barrier', or
>      `set' which is part of a function prologue and sets the stack
>      pointer, sets the frame pointer, or saves a register.  This flag
>      should also be set on an instruction that sets up a temporary
>      register to use in place of the frame pointer.  Stored in the
>      `frame_related' field and printed as `/f'.
>
> I wonder if the insn that stores to (or uses(?)) this temporary register (in
> place of the frame pointer) should also be marked as frame related insn?
>
> So, all the insns in the sequence of
>
> set tmpreg, FP + const
> ...
> store (tmpreg)
>
> should be marked as frame related insns.
>
>
>   
i was not referring to the frame_related flag, though i guess it could
be taken over for this purpose.  Note that the frame_related flag is for
use by the prologue and this is not.  This is just a register that
happens to point into the frame, which i think is only ever created if
you say -fforce-addr.


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #21 from ubizjak at gmail dot com  2007-10-08 08:56 -------
The problem is, that we don't have correct REG_DEP_TRUE expr of the call to
powf after we have pushed ebp+const into temp register.

I'm testing following patch to explow.c:

Index: explow.c
===================================================================
--- explow.c    (revision 129120)
+++ explow.c    (working copy)
@@ -488,7 +488,9 @@ memory_address (enum machine_mode mode, 
     win2:
       x = oldx;
     win:
-      if (flag_force_addr && ! cse_not_expected && !REG_P (x))
+      if (flag_force_addr && ! cse_not_expected && !REG_P (x)
+         && ! reg_mentioned_p (virtual_outgoing_args_rtx, x)
+         && ! reg_mentioned_p (stack_pointer_rtx, x))
        {
          x = force_operand (x, NULL_RTX);
          x = force_reg (Pmode, x);


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ubizjak at gmail dot com


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ubizjak at gmail dot com]

------- Comment #22 from ubizjak at gmail dot com  2007-10-08 12:15 -------
Patch at http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00420.html


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-
                   |                            |patches/2007-
                   |                            |10/msg00420.html


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[Kenneth dot Zadeck at NaturalBridge dot com]

------- Comment #23 from Kenneth dot Zadeck at NaturalBridge dot com  2007-10-08 12:35 -------
Subject: Re:  [4.3 regression]: wrong code with -fforce-addr


Your (a) solution would only paper over the problem: dse assumes that
it can see all of the loads and stores off of the frame in more places
than that call.  So fixing the call dependencies would have just been
the tip of the iceberg.

I like this solution but i am not authorized to approve this patch.

Kenny


-- 


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

[Bug rtl-optimization/33638] [4.3 regression]: wrong code with -fforce-addr

[ebotcazou at gcc dot gnu dot org]

------- Comment #24 from ebotcazou at gcc dot gnu dot org  2007-10-11 05:33 -------
Subject: Bug 33638

Author: ebotcazou
Date: Thu Oct 11 05:33:04 2007
New Revision: 129226

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=129226
Log:
        PR rtl-optimization/33638
        * dse.c (struct insn_info): Remove 'stack_read' field,
        add 'stack_pointer_based' field.
        (record_store): For a store with non-constant base, record
        whether it is stack pointer based.
        (scan_insn): For the call to a const function, remove stack
        pointer based stores from the list of local active stores.
        (scan_reads_nospill): Delete code dealing with const functions.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dse.c


-- 


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

[Bug rtl-optimization/33638] [4.3 regression] wrong code with -O2 -fforce-addr

[ebotcazou at gcc dot gnu dot org]

------- Comment #25 from ebotcazou at gcc dot gnu dot org  2007-10-11 05:39 -------
Probably to be verified on original code.


-- 

ebotcazou at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
           Keywords|                            |wrong-code
         Resolution|                            |FIXED
            Summary|[4.3 regression]: wrong code|[4.3 regression] wrong code
                   |with -fforce-addr           |with -O2 -fforce-addr
   Target Milestone|---                         |4.3.0


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

[Bug rtl-optimization/33638] [4.3 regression] wrong code with -O2 -fforce-addr

[ubizjak at gmail dot com]

------- Comment #26 from ubizjak at gmail dot com  2007-10-11 05:48 -------
Updated patch URL to http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00593.html


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://gcc.gnu.org/ml/gcc-  |http://gcc.gnu.org/ml/gcc-
                   |patches/2007-               |patches/2007-
                   |10/msg00420.html            |10/msg00593.html


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

[Bug rtl-optimization/33638] [4.3 regression] wrong code with -O2 -fforce-addr

[manfred99 at gmx dot ch]

------- Comment #27 from manfred99 at gmx dot ch  2007-10-12 15:28 -------
With gcc of today (patched tree-ssa-forwprop.c to make it bootstrap): 
# gfortran -v
Using built-in specs.
Target: i686-pc-linux-gnu
Configured with: ../gfortran-source/gcc/configure --enable-languages=fortran
--disable-nls --disable-multilib --enable-checking=release
--prefix=/usr/local/gfortran-test
Thread model: posix
gcc version 4.3.0 20071012 (experimental) [trunk revision 129260] (GCC)

The original program works fine now, the bugzilla testcase too, both
with and without "-fforce-addr". I'm not able to trigger this wrong-code 
issue with this new version now.

Note: the testcase gives different output, as meanwhile in gfortran the default 
precision for outputs has been increased. The actual values are the same
though.

Thanks a lot!


-- 

manfred99 at gmx dot ch changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |VERIFIED


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