[RFA:] Fix PR 34773 (4.3 regression)

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

An insn for cris-elf, that is the last of the open-coded negdf2
sequence, is transformed at the end of fwprop1 (before
fwprop_done) to the following.  (NB: whether such sequences
without actual call insns should be wrapped in REG_LIBCALL and
REG_RETVAL is IMHO doubtful, but that's what
emit_no_conflict_block does, so here we are):

(insn 30 28 32 4 vf2.i:20 (set (subreg:SI (reg/v:DF 25 [ value ]) 4)
        (xor:SI (subreg:SI (reg/v:DF 26 [ ap ]) 4)
            (reg:SI 35))) 155 {xorsi3} (expr_list:REG_EQUAL (plus:SI (subreg:SI (reg/v:DF 26 [ ap ]) 4)
            (const_int -2147483648 [0xffffffff80000000]))
        (insn_list:REG_RETVAL 31 (expr_list:REG_NO_CONFLICT (reg/v:DF 26 [ ap ])
                (nil)))))

Spot the error...  Yep, you guessed right. :) The REG_EQUAL and
REG_RETVAL combination is magic: it's supposed to mark the
*whole* setting of the libcall return-value, while the above
obviously only sets the high part, and not even reflecting the
result of the open-coded sequence.  This note combination tricks
cse.c:delete_trivially_dead_insns (called from fwprop_done) and
its callee cse.c:dead_libcall_p into trying to simplify the xor
into a plus (valid; less cost if the value is a constant operand
instead of a register) and then declare victory and rights to
remove *the whole* open-coded "libcall" insn sequence as
redundant, including FWIW, the low-part setting of the result.

There's no doubt about the current REG_EQUAL+REG_RETVAL
significance, as a comment in emit_no_conflict_block says:

      /* Remove any existing REG_EQUAL note from "last", or else it will
	 be mistaken for a note referring to the full contents of the
	 alleged libcall value when found together with the REG_RETVAL
	 note added below.  ...

See also rtl.texi.  For exactly the reason in the comment
(actually, the context of that comment), there's no REG_EQUAL
note added originally, as there's no single insn that can
represent setting of the whole value (though a subreg to use DI
would do, but that's not pursued).

The misplaced REG_EQUAL note is added in fwprop1 by
try_fwprop_subst (from forward_propagate_and_simplify), when it
doesn't follow through with a substitution for an equality,
either because it'd cause an invalid insn or for not being
profitable.  With the REG_EQUAL note, it somehow wants to mark
the insn as having the equality, supposedly hoping to enable
later passes to pursue the substitution.

There's another REG_EQUAL emit-point in fwprop.c, but that
happens for a read-only memory source, so would apparently not
cause spurious combinations with REG_RETVAL.  By a quick look,
at least places in gcse.c look suspicious too, but as we're in
stage 3/regressions only, it didn't seem suitable to fix them.

FWIW, with the patch below, the substitution
(2^31 -> reg; src ^ reg ->dest by: src + 2^31 -> dest)
was later properly performed by gcse1 anyway so there's no
(perceived) loss of performance with regards to this code and
target.

An alternative approach would be to change the callee,
set_unique_reg_note, to not add a REG_EQUAL note when ordered to
set a REG_EQUAL note on an insn where there's already a
REG_RETVAL note.  That approach might be safer for current and
future changes in that most regnote emitters use
set_unique_reg_note, but seems a bit more invasive.  It's
probably the better solution, but pursuable first at the next
stage 1.  Originally the REG_RETVAL note is added last, so maybe
it'd work without further changes, but it'd also require an
audit of places where notes are moved and/or where complete note
reconstruction is done.

I think the combination is documented sufficiently in
doc/rtl.texi, but IMHO there was room for a little improvement
in reg-notes.def.

The test-case is kept cris-specific, as the sign-bit test is
little-endian-specific (or at least float-little-endian) and we
don't have an effective-target qualifier for that AFAICT.

Tested native x86_64-unknown-linux-gnu and crosses to
cris-elf, crisv32-elf, arm-elf, mips-elf, mmix-knuth-mmixware,
cris-axis-linux-gnu and crisv32-axis-linux-gnu.

Ok to commit?

:ADDPATCH rtl-optimization:

gcc/testsuite:
	PR rtl-optimization
	* gcc.target/cris/torture/pr34773.c: New test.

gcc:
	PR rtl-optimization
	* reg-notes.def (EQUAL): Mention significance of combination of
	REG_EQUAL and REG_RETVAL.
	* fwprop.c (try_fwprop_subst): Don't add REG_EQUAL to an
	insn that has a REG_RETVAL.

Index: reg-notes.def
===================================================================
--- reg-notes.def	(revision 130734)
+++ reg-notes.def	(working copy)
@@ -52,7 +52,9 @@ REG_NOTE (EQUIV)
 
 /* Like REG_EQUIV except that the destination is only momentarily
    equal to the specified rtx.  Therefore, it cannot be used for
-   substitution; but it can be used for cse.  */
+   substitution; but it can be used for cse.  Together with a
+   REG_RETVAL note, it means that the insn sets the full contents of
+   the libcall value.  */
 REG_NOTE (EQUAL)
 
 /* This insn copies the return-value of a library call out of the hard
Index: fwprop.c
===================================================================
--- fwprop.c	(revision 130734)
+++ fwprop.c	(working copy)
@@ -729,9 +729,12 @@ try_fwprop_subst (struct df_ref *use, rt
     {
       cancel_changes (0);
 
-      /* Can also record a simplified value in a REG_EQUAL note, making a
-	 new one if one does not already exist.  */
-      if (set_reg_equal)
+      /* Can also record a simplified value in a REG_EQUAL note,
+	 making a new one if one does not already exist.
+	 Don't do this if the insn has a REG_RETVAL note, because the
+	 combined presence means that the REG_EQUAL note refers to the
+	 (full) contents of the libcall value.  */
+      if (set_reg_equal && !find_reg_note (insn, REG_RETVAL, NULL_RTX))
 	{
 	  if (dump_file)
 	    fprintf (dump_file, " Setting REG_EQUAL note\n");

Index: gcc.target/cris/torture/pr34773.c
===================================================================
--- gcc.target/cris/torture/pr34773.c	(revision 0)
+++ gcc.target/cris/torture/pr34773.c	(revision 0)
@@ -0,0 +1,74 @@
+/* { dg-do run } */
+union double_union
+{
+  double d;
+  int i[2];
+};
+void _dtoa_r (double) __attribute__ ((__noinline__));
+void _vfprintf_r (double) __attribute__ ((__noinline__));
+void
+__sprint_r(int);
+void
+_vfprintf_r(double da)
+{
+ double ffp = da;
+ double value = ffp;
+ union double_union tmp;
+
+ tmp.d = value;
+
+ if ((tmp.i[1]) & ((unsigned)0x80000000L)) {
+   value = -value;
+ }
+
+ _dtoa_r (value);
+
+ if (ffp != 0)
+   __sprint_r(value == 0);
+ asm ("");
+}
+
+
+double dd = -.012;
+double ff = .012;
+
+void exit (int) __attribute__ ((__noreturn__));
+void abort (void) __attribute__ ((__noreturn__));
+void *memset(void *s, int c, __SIZE_TYPE__ n);
+void _dtoa_r (double d)
+{
+  if (d != ff)
+    abort ();
+  asm ("");
+}
+
+void __sprint_r (int i)
+{
+  if (i != 0)
+    abort ();
+  asm ("");
+}
+
+int clearstack (void) __attribute__ ((__noinline__));
+int clearstack (void)
+{
+  char doodle[128];
+  memset (doodle, 0, sizeof doodle);
+  asm volatile ("" : : "g" (doodle) : "memory");
+  return doodle[127];
+}
+
+void doit (void) __attribute__ ((__noinline__));
+void doit (void)
+{
+  _vfprintf_r (dd);
+  _vfprintf_r (ff);
+  asm ("");
+}
+
+int main(void)
+{
+  clearstack ();
+  doit ();
+  exit (0);
+}

brgds, H-P