Re: [PATCH, ARM] Don't copy uncopyable instructions in gcse.c

[Julian Brown <julian at codesourcery dot com>]

Steven Bosscher wrote:

> So how does your patch make a difference?  local copy propagation does
> not use hash_scan_set AFAIK.

> But if your problem is really caused in local-cprop, then you really
> are just papering over the real bug...

I think I messed up my rationale for the patch: the duplication of the
instruction actually happens during PRE, not local cprop. Extracts
of the diff of the relevant parts of the 142r.pre dump file show
before/after my patch, e.g.:

 Index 3 (hash value 4)
-  (unspec:SI [
-        (const_int 4 [0x4])
-        (const_int 0 [0x0])
-    ] 4)

-PRE: edge (5,7), copy expression 3

-PRE: edge (6,7), copy expression 3

Is that more believable?

On Thu, 21 May 2009 10:49:02 +0200
Eric Botcazou <ebotcazou@adacore.com> wrote:

> >     * gcse.c (target.h): Include.
> >     (hash_scan_set): Don't make copies of instructions the target
> > deems uncopyable.
> >     * config/arm/arm.c (arm_cannot_copy_insn_p): Check we have an
> >     instruction.
> 
> Don't change the interface of the hook, i.e. verify that it's not
> NULL and the insn is INSN_P before calling it.  See
> cfg_layout_can_duplicate_bb_p.

Yeah, that makes more sense. Fixed.

> Why do you need the second hunk in hash_scan_set?  Does the source
> verify gcse_constant_p?

It seems to be unnecessary to fix my build, so I've removed it. I
figured (perhaps incorrectly) that there was a chance of other code
sequences (or other future uncopyable instructions) triggering that
branch.

> I think that this should be prevented by want_to_gcse_p, maybe by
> verifying in can_assign_to_reg_without_clobbers_p that test_insn is
> not cannot_copy_insn_p or devising a similar helper.  Steven, what do
> you think?

I've not done that, but is the current version reasonable? (Re-testing).

Thanks,

Julian

ChangeLog

    gcc/
    * gcse.c (hash_scan_set): Don't make copies of instructions the
    target deems uncopyable.

Index: gcc/gcse.c
===================================================================
--- gcc/gcse.c	(revision 147741)
+++ gcc/gcse.c	(working copy)
@@ -169,6 +169,7 @@ along with GCC; see the file COPYING3.  
 #include "hashtab.h"
 #include "df.h"
 #include "dbgcnt.h"
+#include "target.h"
 
 /* Propagate flow information through back edges and thus enable PRE's
    moving loop invariant calculations out of loops.
@@ -1349,7 +1350,8 @@ hash_scan_set (rtx pat, rtx insn, struct
 	     REG_EQUIV notes and if the argument slot is used somewhere
 	     explicitly, it means address of parameter has been taken,
 	     so we should not extend the lifetime of the pseudo.  */
-	  && (note == NULL_RTX || ! MEM_P (XEXP (note, 0))))
+	  && (note == NULL_RTX || ! MEM_P (XEXP (note, 0)))
+	  && ! (INSN_P (insn) && targetm.cannot_copy_insn_p (insn)))
 	{
 	  /* An expression is not anticipatable if its operands are
 	     modified before this insn or if this is not the only SET in