This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: CSE bug (gcc.c-torture/execute/980605-1.c)
- To: davem at dm dot cobaltmicro dot com (David S. Miller)
- Subject: Re: CSE bug (gcc.c-torture/execute/980605-1.c)
- From: Carlo Wood <carlo at runaway dot xs4all dot nl>
- Date: Mon, 6 Jul 1998 16:49:04 +0200 (CEST)
- Cc: egcs at cygnus dot com (egcs at cygnus dot com)
I have investigating the same bug for a few days, and I think I have evidence
that this patch can't be correct.
This is a preliminary post, I am not done with my research at all.
But I thought it would be better to tell you that I am working on this too -
and that I'd like to do a little more research before Davids patch is added :).
| This one is nasty and has been in GCC for a long time. When it
| doesn't generate incorrect code, the bug prevents libcalls and
| functions marked with attribute const from being optimized away in
| most cases. ;-(
|
| For reference since the test case is quite small:
|
| ========================================
| static int f(int) __attribute__((const));
| int main()
| {
| int f1, f2, x;
| x = 1; f1 = f(x);
| x = 2; f2 = f(x);
| if (f1 != 1 || f2 != 2)
| abort ();
| exit (0);
| }
| static int f(int x) { return x; }
| ========================================
|
| This generates the following RTL on 32-bit Sparc targets.
|
| ========================================
| (insn 9 6 11 (set (reg/v:SI 107)
| (const_int 1)) -1 (nil)
| (nil))
|
| (insn 11 9 13 (set (reg:SI 8 %o0)
| (reg/v:SI 107)) -1 (nil)
| (insn_list:REG_LIBCALL 15 (nil)))
|
| (call_insn/u 13 11 15 (parallel[
| (set (reg:SI 8 %o0)
| (call (mem:SI (symbol_ref:SI ("f")))
| (const_int 0)))
| (clobber (reg:SI 15 %o7))
| ] ) -1 (nil)
| (nil)
| (expr_list (use (reg:SI 8 %o0))
| (nil)))
|
| (insn 15 13 17 (set (reg:SI 108)
| (reg:SI 8 %o0)) -1 (nil)
| (insn_list:REG_RETVAL 11 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI ("f"))
| (expr_list (reg/v:SI 107)
| (nil)))
| (nil))))
| ========================================
|
| The first run of CSE removes the set of REG 107 in insn 9 and changes
| insn 11 to be (set (reg:SI %o0) (const_int 1))
|
| This is fine, except for the fact that the REG_EQUAL information at
| the end of the libcall block at insn 15 is now completely bogus.
|
| The fix is to rewrite such REG_EQUAL lists with the replacements made
| when CSE performs such changes within a libcall block.
|
| Here is my attempt at a fix, comments?
|
| Fri Jul 3 23:04:59 1998 David S. Miller <davem@pierdol.cobaltmicro.com>
|
| * cse.c (rewrite_libcall_equivs): New function.
| (cse_insn): Call it when substituting the source of a SET within a
| libcall block.
|
| --- cse.c.~1~ Thu Jul 2 16:32:44 1998
| +++ cse.c Fri Jul 3 22:46:43 1998
| @@ -6098,6 +6098,33 @@
| merge_equiv_classes (op0_elt, op1_elt);
| last_jump_equiv_class = op0_elt;
| }
| +
| +/* Rewrite all REG_EQUIVs mentioning OLD_SRC to mention NEW_SRC at
| + the end of libcall block starting at INSN. */
| +
| +static void
| +rewrite_libcall_equivs (insn, old_src, new_src)
| + rtx insn;
| + rtx old_src;
| + rtx new_src;
| +{
| + rtx note;
| +
| + while (insn != NULL_RTX)
| + {
| + if (find_reg_note (insn, REG_RETVAL, NULL_RTX))
| + break;
| + insn = NEXT_INSN (insn);
| + }
| +
| + if (insn == NULL_RTX)
| + abort();
| +
| + note = find_reg_note (insn, REG_EQUAL, NULL_RTX);
| + if (note != NULL_RTX)
| + replace_rtx (XEXP (note, 0), old_src, new_src);
| +}
| +
|
| /* CSE processing for one instruction.
| First simplify sources and addresses of all assignments
| @@ -6929,12 +6956,18 @@
| /* Look for a substitution that makes a valid insn. */
| else if (validate_change (insn, &SET_SRC (sets[i].rtl), trial, 0))
| {
| + rtx new_src = canon_reg (SET_SRC (sets[i].rtl), insn);
| +
| + /* If we are in a libcall block, we must make sure we did
| + not change the meaning of any REG_EQUIVs living at the
| + end of it. */
| + if (in_libcall_block)
| + rewrite_libcall_equivs (insn, sets[i].src, new_src);
| +
| /* The result of apply_change_group can be ignored; see
| canon_reg. */
|
| - validate_change (insn, &SET_SRC (sets[i].rtl),
| - canon_reg (SET_SRC (sets[i].rtl), insn),
| - 1);
| + validate_change (insn, &SET_SRC (sets[i].rtl), new_src, 1);
| apply_change_group ();
| break;
| }
It may be important that I am trying a slightly different test case:
========================================
980505-1.i:
========================================
# 1 "/usr/src/egcs/egcs-cvs/gcc/testsuite/gcc.c-torture/execute/980505-1.c"
int f(int) __attribute__((const));
int main()
{
int f1, f2, x;
x = 1;
f1 = f(x);
x = 2;
f2 = f(x);
if (f1 == 1)
if (f2 == 2)
exit(0);
abort();
}
========================================
Note that I left away completely the definition of f().
Then I compile this by running:
/usr/src/egcs/egcs-cvs-objdir/gcc/cc1 980505-1.i -quiet -dumpbase 980505-1.c -O -frerun-cse-after-loop -o 980505-1.s
I added some debug calls in the code that print out the RTL.
The just *before* the `if' in:
| /* Look for a substitution that makes a valid insn. */
| else if (validate_change (insn, &SET_SRC (sets[i].rtl), trial, 0))
It looks like:
-----START-------RTL before `if'---------------------------------
(note 2 0 3 "" NOTE_INSN_DELETED)
(note 3 2 4 "" NOTE_INSN_FUNCTION_BEG)
(note 4 3 6 "" NOTE_INSN_DELETED)
(note 6 4 12 0 NOTE_INSN_BLOCK_BEG)
(insn 12 6 14 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
(const_int 1)) 50 {movsi-2} (nil)
(insn_list:REG_LIBCALL 18 (nil)))
(call_insn/u 14 12 16 (set (reg:SI 0 %eax)
(call (mem:QI (symbol_ref:SI ("f")))
(const_int 4))) -1 (nil)
(nil)
(nil))
(insn 16 14 18 (set (reg:SI 7 %esp)
(plus:SI (reg:SI 7 %esp)
(const_int 4))) 143 {addsi3+1} (nil)
(nil))
(insn 18 16 26 (set (reg:SI 24)
(reg:SI 0 %eax)) 54 {movsi+2} (nil)
(insn_list:REG_RETVAL 12 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI ("f"))
(expr_list (reg/v:SI 23)
(nil)))
(nil))))
(insn 26 18 28 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
(const_int 2)) 50 {movsi-2} (nil)
(insn_list:REG_LIBCALL 32 (nil)))
(call_insn/u 28 26 30 (set (reg:SI 0 %eax)
(call (mem:QI (symbol_ref:SI ("f")))
(const_int 4))) -1 (nil)
(nil)
(nil))
(insn 30 28 32 (set (reg:SI 7 %esp)
(plus:SI (reg:SI 7 %esp)
(const_int 4))) 143 {addsi3+1} (nil)
(nil))
(insn 32 30 36 (set (reg:SI 25)
(reg:SI 24)) 54 {movsi+2} (nil)
(insn_list:REG_RETVAL 26 (expr_list:REG_EQUAL (expr_list (symbol_ref:SI ("f"))
(expr_list (reg/v:SI 23)
(nil)))
(nil))))
(insn 36 32 37 (set (cc0)
(compare (reg:SI 24)
(const_int 1))) 12 {cmpsi_1} (nil)
(nil))
(jump_insn 37 36 39 (set (pc)
(if_then_else (ne (cc0)
(const_int 0))
(label_ref 50)
(pc))) 282 {bne+1} (nil)
(nil))
(insn 39 37 40 (set (cc0)
(compare (reg:SI 24)
(const_int 2))) 12 {cmpsi_1} (nil)
(nil))
(jump_insn 40 39 43 (set (pc)
(if_then_else (ne (cc0)
(const_int 0))
(label_ref 50)
(pc))) 282 {bne+1} (nil)
(nil))
(insn 43 40 45 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
(const_int 0)) 50 {movsi-2} (nil)
(nil))
(call_insn 45 43 46 (set (reg:SI 0 %eax)
(call (mem:QI (symbol_ref:SI ("exit")))
(const_int 4))) -1 (nil)
(nil)
(nil))
(barrier 46 45 50)
(code_label 50 46 53 2 "")
(call_insn 53 50 54 (parallel[
(set (reg:SI 0 %eax)
(call (mem:QI (symbol_ref:SI ("abort")))
(const_int 0)))
(set (reg:SI 7 %esp)
(plus:SI (reg:SI 7 %esp)
(const_int 0)))
] ) 331 {call_value-1} (nil)
(nil)
(nil))
(barrier 54 53 56)
(note 56 54 0 0 NOTE_INSN_BLOCK_END)
--------END------RTL before `if'---------------------------------
While the RTL directly after the '{' of this `if' looks like:
-----START-------RTL directly after {'---------------------------
...
(insn 39 37 40 (set (cc0)
(compare (reg:SI 24)
(const_int 2))) 12 {cmpsi_1} (nil)
(nil))
(jump_insn 40 39 43 (set (pc)
(label_ref 50)) 309 {jump} (nil)
(nil))
...
-----END---------RTL directly after {'---------------------------
For convience, the diff between these two looks like:
-----START-diff------------------------
*** rtl-899735463.000868327-cse.c:6956-while.5 Mon Jul 6 16:31:03 1998
--- rtl-899735463.000871075-cse.c:6996-while.9 Mon Jul 6 16:31:03 1998
***************
*** 68,77 ****
(nil))
(jump_insn 40 39 43 (set (pc)
! (if_then_else (ne (cc0)
! (const_int 0))
! (label_ref 50)
! (pc))) 282 {bne+1} (nil)
(nil))
(insn 43 40 45 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
--- 68,74 ----
(nil))
(jump_insn 40 39 43 (set (pc)
! (label_ref 50)) 309 {jump} (nil)
(nil))
(insn 43 40 45 (set (mem:SI (pre_dec:SI (reg:SI 7 %esp)))
-----END---diff------------------------
I think that changing the conditional jump into an unconditional jump
is wrong - and hence, the bug already occurs before where you patched
something.
Note that the result of this change is something like:
main:
pushl %ebp
movl %esp,%ebp
pushl $1
call f
addl $4,%esp
cmpl $1,%eax
jne .L2
jmp .L2
.p2align 4,,7
.L2:
call abort
(This the output WITHOUT "jump2" optimisation, although the
second call to `f' is already removed; I yet have to find
a way to reliable produce a .s file from any point during
the optisation (I understand .s better than rtl :/)).
--
Carlo Wood <carlo@runaway.xs4all.nl>