Re: [PATCH] Use fld b; fld a; instead of fld a; fld b; fxch %st(1) in reg-stack (PR target/70465)

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Jan 25, 2017 at 02:43:34PM -0700, Jeff Law wrote:
> > 2017-01-25  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/70465
> > 	* reg-stack.c (emit_swap_insn): Instead of fld a; fld b; fxchg %st(1);
> > 	emit fld b; fld a; if possible.
> > 
> > 	* gcc.target/i386/pr70465.c: New test.
> So please comment on the general approach you're taking here.  I have a
> pretty good sense of what you're doing, mostly because I pondered something
> similar.  But I doubt others coming across the code would see the overall
> structure as quickly.

Does the following updated patch explain it sufficiently?

> > +	  if (i2 != NULL_RTX
> > +	      && (i2set = single_set (i2)) != NULL_RTX)
> > +	    {
> > +	      /* Instead of fld a; fld b; fxch %st(1); just
> > +		 use fld b; fld a; if possible.  */
> > +	      rtx i2dest = *get_true_reg (&SET_DEST (i2set));
> > +	      if (REG_P (i2dest)
> > +		  && REGNO (i2dest) == FIRST_STACK_REG
> > +		  && MEM_P (SET_SRC (i2set))
> > +		  && !side_effects_p (SET_SRC (i2set))
> > +		  && !modified_between_p (SET_SRC (i1set), i2, i1))
> And here we're trying to verify that the insn found above (I2) is pushing
> another value onto the FP register stack and that the value in I2 is not
> modified between I1 and I2.

No, that last call (as I've tried to explain in the new comment) wants
to ensure that there are no stores in between i2 and i1 that might
alias with the second load's memory (then it would be invalid to move it
before i2) and that the address of the memory doesn't depend on something
set after i2.

2017-01-25  Jakub Jelinek  <jakub@redhat.com>

	PR target/70465
	* reg-stack.c (emit_swap_insn): Instead of fld a; fld b; fxchg %st(1);
	emit fld b; fld a; if possible.

	* gcc.target/i386/pr70465.c: New test.

--- gcc/reg-stack.c.jj	2017-01-25 17:17:46.086112137 +0100
+++ gcc/reg-stack.c	2017-01-25 22:58:00.403259702 +0100
@@ -887,6 +887,77 @@ emit_swap_insn (rtx_insn *insn, stack_pt
 	  && REG_P (i1src) && REGNO (i1src) == FIRST_STACK_REG
 	  && find_regno_note (i1, REG_DEAD, FIRST_STACK_REG) == NULL_RTX)
 	return;
+
+      /* Instead of
+	   fld a
+	   fld b
+	   fxch %st(1)
+	 just use
+	   fld b
+	   fld a
+         if possible.  */
+
+      if (REG_P (i1dest)
+	  && REGNO (i1dest) == FIRST_STACK_REG
+	  && MEM_P (SET_SRC (i1set))
+	  && !side_effects_p (SET_SRC (i1set))
+	  && hard_regno == FIRST_STACK_REG + 1
+	  && i1 != BB_HEAD (current_block))
+	{
+	  /* i1 is the last insn that involves stack regs before insn, and
+	     is known to be a load without other side-effects, i.e. fld b
+	     in the above comment.  */
+	  rtx_insn *i2 = NULL;
+	  rtx i2set;
+	  rtx_insn *tmp = PREV_INSN (i1);
+	  rtx_insn *limit = PREV_INSN (BB_HEAD (current_block));
+	  /* Find the previous insn involving stack regs, but don't pass a
+	     block boundary.  */
+	  while (tmp != limit)
+	    {
+	      if (LABEL_P (tmp)
+		  || CALL_P (tmp)
+		  || NOTE_INSN_BASIC_BLOCK_P (tmp)
+		  || (NONJUMP_INSN_P (tmp)
+		      && stack_regs_mentioned (tmp)))
+		{
+		  i2 = tmp;
+		  break;
+		}
+	      tmp = PREV_INSN (tmp);
+	    }
+	  if (i2 != NULL_RTX
+	      && (i2set = single_set (i2)) != NULL_RTX)
+	    {
+	      rtx i2dest = *get_true_reg (&SET_DEST (i2set));
+	      /* If the last two insns before insn that involve
+		 stack regs are loads, where the latter (i1)
+		 pushes onto the register stack and thus
+		 moves the value from the first load (i2) from
+		 %st to %st(1), consider swapping them.  */
+	      if (REG_P (i2dest)
+		  && REGNO (i2dest) == FIRST_STACK_REG
+		  && MEM_P (SET_SRC (i2set))
+		  /* Ensure i2 doesn't have other side-effects.  */
+		  && !side_effects_p (SET_SRC (i2set))
+		  /* And that the two instructions can actually be
+		     swapped, i.e. there shouldn't be any stores
+		     in between i2 and i1 that might alias with
+		     the i1 memory, and the memory address can't
+		     use registers set in between i2 and i1.  */
+		  && !modified_between_p (SET_SRC (i1set), i2, i1))
+		{
+		  /* Move i1 (fld b above) right before i2 (fld a
+		     above.  */
+		  remove_insn (i1);
+		  SET_PREV_INSN (i1) = NULL_RTX;
+		  SET_NEXT_INSN (i1) = NULL_RTX;
+		  set_block_for_insn (i1, NULL);
+		  emit_insn_before (i1, i2);
+		  return;
+		}
+	    }
+	}
     }
 
   /* Avoid emitting the swap if this is the first register stack insn
--- gcc/testsuite/gcc.target/i386/pr70465.c.jj	2017-01-25 22:49:21.986853620 +0100
+++ gcc/testsuite/gcc.target/i386/pr70465.c	2017-01-25 22:49:21.985853633 +0100
@@ -0,0 +1,12 @@
+/* PR target/70465 */
+/* { dg-do compile { target ia32 } } */
+/* { dg-options "-O2 -mfpmath=387 -fomit-frame-pointer" } */
+/* { dg-final { scan-assembler-not "fxch\t%st.1" } } */
+
+double
+atan2 (double y, double x)
+{
+  double res = 0.0;
+  asm ("fpatan" : "=t" (res) : "u" (y), "0" (x) : "st(1)");
+  return res;
+}


	Jakub