Re: [m68k 10/13] cleanup fp compare

[Roman Zippel <zippel at linux-m68k dot org>]

Hi,

On Mon, 5 Feb 2007, Richard Sandiford wrote:

> > What difference does it make at this point?
> 
> The difference is that conditions are AIUI canonicalised at the tree level
> in such a way that the second operand is likely to the non-constant once.

Ah, I forgot about this.

> >> is in fp_src_operand.  All I'm really saying is that fp_src_operand
> >> should reject constants if TARGET_COLDFIRE_FPU.
> >
> > Any constants? Why?
> 
> Yes, and because the ColdFire FPU's comparison insns don't allow
> constant operands.  They're not as rich as the 68881 insns.

Then it makes sense of course.

> I don't think it has to be "one or the other".  The standard idiom,
> as used in the move patterns on most RISC targets, is to use the most
> permissive applicable predicate for both operands, then use both the
> constraints _and_ the insn condition to force one of the operands to be
> a register.  That way you get the desired restrictions before, during,
> and after reload.  E.g.:
> 
> ---------------------------------------------------------------------------
> (define_insn "*cmp<mode>_cf"
>   [(set (cc0)
>         (compare (match_operand:FP 0 "fp_src_operand" "f,<FP:dreg><Q>U")
>                  (match_operand:FP 1 "fp_src_operand" "f<FP:dreg><Q>U,f")))]
>   "TARGET_COLDFIRE_FPU
>    && (register_operand (operands[0], VOIDmode)
>        || register_operand (operands[1], VOIDmode))"
> {
>   cc_status.flags = CC_IN_68881;
>   if (FP_REG_P (operands[0]))
>     {
>       if (FP_REG_P (operands[1]))
>         return "fcmp%.d %1,%0";
>       else
>         return "fcmp%.<FP:prec> %f1,%0";
>     }
>   cc_status.flags |= CC_REVERSED;
>   return "fcmp%.<FP:prec> %f0,%1";
> })
> ---------------------------------------------------------------------------
> 
> and similar for the m68k.  (Untested.)

Ok, below is an updated patch, which also rejects all constants for 
ColdFire. (Ligthly tested.)

bye, Roman

---
 gcc/config/m68k/m68k.c        |    7 ++++
 gcc/config/m68k/m68k.md       |   62 +++++++++++++++---------------------------
 gcc/config/m68k/predicates.md |   11 +++++++
 3 files changed, 41 insertions(+), 39 deletions(-)

Index: egcs/gcc/config/m68k/m68k.c
===================================================================
--- egcs.orig/gcc/config/m68k/m68k.c
+++ egcs/gcc/config/m68k/m68k.c
@@ -2875,6 +2875,13 @@ notice_update_cc (rtx exp, rtx insn)
   if (((cc_status.value1 && FP_REG_P (cc_status.value1))
        || (cc_status.value2 && FP_REG_P (cc_status.value2))))
     cc_status.flags = CC_IN_68881;
+  if (cc_status.value2 && GET_CODE (cc_status.value2) == COMPARE
+      && GET_MODE_CLASS (GET_MODE (XEXP (cc_status.value2, 0))) == MODE_FLOAT)
+    {
+      cc_status.flags = CC_IN_68881;
+      if (!FP_REG_P (XEXP (cc_status.value2, 0)))
+	cc_status.flags |= CC_REVERSED;
+    }
 }
 
 const char *
Index: egcs/gcc/config/m68k/m68k.md
===================================================================
--- egcs.orig/gcc/config/m68k/m68k.md
+++ egcs/gcc/config/m68k/m68k.md
@@ -406,50 +406,34 @@
 
 (define_expand "cmp<mode>"
   [(set (cc0)
-	(compare (match_operand:FP 0 "general_operand" "")
-		 (match_operand:FP 1 "general_operand" "")))]
+	(compare (match_operand:FP 0 "register_operand" "")
+		 (match_operand:FP 1 "fp_src_operand" "")))]
   "TARGET_HARD_FLOAT"
-{
-  m68k_last_compare_had_fp_operands = 1;
-  if (TARGET_COLDFIRE && !reload_completed)
-    operands[1] = force_reg (<MODE>mode, operands[1]);
-})
+  "m68k_last_compare_had_fp_operands = 1;")
 
-(define_insn "cmp<mode>_68881"
+(define_insn "*cmp<mode>_68881"
   [(set (cc0)
-	(compare (match_operand:FP 0 "general_operand" "f,m<FP:const>")
-		 (match_operand:FP 1 "general_operand" "f<FP:dreg>m<FP:const>,f")))]
-  "TARGET_68881"
-{
-  cc_status.flags = CC_IN_68881;
-  if (FP_REG_P (operands[0]))
-    {
-      if (FP_REG_P (operands[1]))
-	return "fcmp%.x %1,%0";
-      else
-        return "fcmp%.<FP:prec> %f1,%0";
-    }
-  cc_status.flags |= CC_REVERSED;
-  return "fcmp%.<FP:prec> %f0,%1";
-})
+	(compare (match_operand:FP 0 "fp_src_operand" "f,f,<FP:dreg>mF")
+		 (match_operand:FP 1 "fp_src_operand" "f,<FP:dreg>mF,f")))]
+  "TARGET_68881
+   && (register_operand (operands[0], <MODE>mode)
+       || register_operand (operands[1], <MODE>mode))"
+  "@
+   fcmp%.x %1,%0
+   fcmp%.<FP:prec> %f1,%0
+   fcmp%.<FP:prec> %0,%f1")
 
-(define_insn "cmp<mode>_cf"
+(define_insn "*cmp<mode>_cf"
   [(set (cc0)
-	(compare (match_operand:FP 0 "general_operand" "f,<FP:dreg><Q>U")
-		 (match_operand:FP 1 "general_operand" "f<FP:dreg><Q>U,f")))]
-  "TARGET_COLDFIRE_FPU"
-{
-  cc_status.flags = CC_IN_68881;
-  if (FP_REG_P (operands[0]))
-    {
-      if (FP_REG_P (operands[1]))
-	return "fcmp%.d %1,%0";
-      else
-	return "fcmp%.<FP:prec> %f1,%0";
-    }
-  cc_status.flags |= CC_REVERSED;
-  return "fcmp%.<FP:prec> %f0,%1";
-})
+	(compare (match_operand:FP 0 "fp_src_operand" "f,f,<FP:dreg><Q>U")
+		 (match_operand:FP 1 "fp_src_operand" "f,<FP:dreg><Q>U,f")))]
+  "TARGET_COLDFIRE_FPU
+   && (register_operand (operands[0], <MODE>mode)
+       || register_operand (operands[1], <MODE>mode))"
+  "@
+   fcmp%.d %1,%0
+   fcmp%.<FP:prec> %f1,%0
+   fcmp%.<FP:prec> %0,%f1")
 
 ;; Recognizers for btst instructions.
 
Index: egcs/gcc/config/m68k/predicates.md
===================================================================
--- egcs.orig/gcc/config/m68k/predicates.md
+++ egcs/gcc/config/m68k/predicates.md
@@ -181,3 +181,14 @@
 (define_predicate "pre_dec_operand"
   (and (match_code "mem")
        (match_test "GET_CODE (XEXP (op, 0)) == PRE_DEC")))
+
+;; Special case of general_src_operand, which rejects a few fp
+;; constants (which we prefer in registers) before reload.
+
+(define_predicate "fp_src_operand"
+  (match_operand 0 "general_src_operand")
+{
+  return !CONSTANT_P (op) || (!TARGET_COLDFIRE_FPU
+			      && !standard_68881_constant_p (op))
+	 || reload_in_progress || reload_completed;
+})