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.
Created attachment 14288 [details] Source code of affected function
Created attachment 14289 [details] Assembler code of original code
Created attachment 14290 [details] Assembler code when commenting line 315, works
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 ;)
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.
> 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.
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.
Created attachment 14300 [details] test case
> 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'
(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.
But powf is pure/const, so the call is not a use.
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.
(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.
The testcase works for me, that is, it produces the expected output good.out.
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
(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
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
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
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.
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.
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);
Patch at http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00420.html
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
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
Probably to be verified on original code.
Updated patch URL to http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00593.html
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!