This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix GCSE and CSE with REG_EQUIV
On Wed, Apr 04, 2001 at 05:03:32PM +0100, Bernd Schmidt wrote:
> On Wed, 4 Apr 2001, Jakub Jelinek wrote:
>
> > >From what I understood, at GCSE and CSE time, the only REG_EQUIV's in the
> > chain are those for parameters, and unless the parameters addresses have
> > been taken, they will be always accessed through their pseudos not the MEMs,
> > so pushing the REG_EQUIV sets into CSE/GCSE hashes IMHO only triggers the
> > bad cases when it is not safe to optimize it and should never trigger any
> > actual valid gains.
>
> gcse may need fixing, but why isn't this piece of code in cse sufficient?
Two reasons:
1) it is enough to have REG_N_SETS (REGNO (dest) == 1 for things to be
miscompiled - there is just one set of the REG_EQUIVed pseudo which is
never used again and the problem is if CSE decides to use it again
2) it calls find_reg_note (insn, REG_EQUIV, src) but that does not work
after addressof changes - although XEXP (link, 0) of the REG_EQUIV note
is rtx_equal_p (, src), it is different pointer and find_reg_note
compares pointers
I've just made two changes in the test for setting src_volatile and the
testcase from c-torture which was miscompiled is fixed by this as well.
Does it look better?
I'm firing off a bootstrap/regression test overnight to test it.
2001-04-04 Jakub Jelinek <jakub@redhat.com>
* gcse.c (gcse_main): Fix comment typo.
(delete_null_pointer_check): Likewise.
(hash_scan_set): Don't consider sets with REG_EQUIV MEM notes.
* cse.c (cse_insn): Likewise.
* function.c (fixup_var_refs_insns_with_hash): The sequence is
toplevel.
* gcc.c-torture/execute/20010403-1.c: New test.
--- gcc/testsuite/gcc.c-torture/execute/20010403-1.c.jj Tue Apr 3 11:42:23 2001
+++ gcc/testsuite/gcc.c-torture/execute/20010403-1.c Thu Apr 5 00:02:18 2001
@@ -0,0 +1,36 @@
+void b (int *);
+void c (int, int);
+void d (int);
+
+int e;
+
+void a (int x, int y)
+{
+ int f = x ? e : 0;
+ int z = y;
+
+ b (&y);
+ c (z, y);
+ d (f);
+}
+
+void b (int *y)
+{
+ (*y)++;
+}
+
+void c (int x, int y)
+{
+ if (x == y)
+ abort ();
+}
+
+void d (int x)
+{
+}
+
+int main (void)
+{
+ a (0, 0);
+ exit (0);
+}
--- gcc/gcse.c.jj Wed Apr 4 22:55:21 2001
+++ gcc/gcse.c Thu Apr 5 00:04:37 2001
@@ -681,7 +681,7 @@ gcse_main (f, file)
a high connectivity will take a long time and is unlikely to be
particularly useful.
- In normal circumstances a cfg should have about twice has many edges
+ In normal circumstances a cfg should have about twice as many edges
as blocks. But we do not want to punish small functions which have
a couple switch statements. So we require a relatively large number
of basic blocks and the ratio of edges to blocks to be high. */
@@ -1964,7 +1964,14 @@ hash_scan_set (pat, insn, set_p)
/* Is SET_SRC something we want to gcse? */
&& want_to_gcse_p (src)
/* Don't CSE a nop. */
- && ! set_noop_p (pat))
+ && ! set_noop_p (pat)
+ /* Don't GCSE if it has attached REG_EQUIV note.
+ At this point this only function parameters should have
+ REG_EQUIV notes and if the argument slot is used somewhere
+ explicitely, it means address of parameter has been taken,
+ so we should not extend the lifetime of the pseudo. */
+ && ((note = find_reg_note (insn, REG_EQUIV, NULL_RTX)) == 0
+ || GET_CODE (XEXP (note, 0)) != MEM))
{
/* An expression is not anticipatable if its operands are
modified before this insn or if this is not the only SET in
@@ -5164,7 +5171,7 @@ delete_null_pointer_checks (f)
a high connectivity will take a long time and is unlikely to be
particularly useful.
- In normal circumstances a cfg should have about twice has many edges
+ In normal circumstances a cfg should have about twice as many edges
as blocks. But we do not want to punish small functions which have
a couple switch statements. So we require a relatively large number
of basic blocks and the ratio of edges to blocks to be high. */
--- gcc/cse.c.jj Wed Apr 4 18:09:28 2001
+++ gcc/cse.c Thu Apr 5 00:30:18 2001
@@ -5067,18 +5067,16 @@ cse_insn (insn, libcall_insn)
sets[i].src_in_memory = hash_arg_in_memory;
/* If SRC is a MEM, there is a REG_EQUIV note for SRC, and DEST is
- a pseudo that is set more than once, do not record SRC. Using
- SRC as a replacement for anything else will be incorrect in that
- situation. Note that this usually occurs only for stack slots,
- in which case all the RTL would be referring to SRC, so we don't
- lose any optimization opportunities by not having SRC in the
- hash table. */
+ a pseudo, do not record SRC. Using SRC as a replacement for
+ anything else will be incorrect in that situation. Note that
+ this usually occurs only for stack slots, in which case all the
+ RTL would be referring to SRC, so we don't lose any optimization
+ opportunities by not having SRC in the hash table. */
if (GET_CODE (src) == MEM
- && find_reg_note (insn, REG_EQUIV, src) != 0
+ && find_reg_note (insn, REG_EQUIV, NULL_RTX) != 0
&& GET_CODE (dest) == REG
- && REGNO (dest) >= FIRST_PSEUDO_REGISTER
- && REG_N_SETS (REGNO (dest)) != 1)
+ && REGNO (dest) >= FIRST_PSEUDO_REGISTER)
sets[i].src_volatile = 1;
#if 0
--- gcc/function.c.jj Thu Apr 5 00:08:49 2001
+++ gcc/function.c Thu Apr 5 00:09:42 2001
@@ -1680,7 +1680,7 @@ fixup_var_refs_insns_with_hash (ht, var,
rtx insn = XEXP (insn_list, 0);
if (INSN_P (insn))
- fixup_var_refs_insn (insn, var, promoted_mode, unsignedp, 0);
+ fixup_var_refs_insn (insn, var, promoted_mode, unsignedp, 1);
insn_list = XEXP (insn_list, 1);
}
Jakub