Re: [m68k 10/13] cleanup fp compare

[Richard Sandiford <richard at codesourcery dot com>]

I like the idea here, but I'm a little worried about some details.

zippel@linux-m68k.org writes:
>  (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;")

It looks to me like you deleted the force_reg here without replacing
it with something else.  This ColdFire code seems to have been a
half-measure that goes some way to doing what your patch does,
but it only did it for ColdFire, and (wrongly) didn't enforce the
same condition in the insn itself.  (And I've no idea why it
checked reload_completed -- the compare optabs shouldn't be called
after reload.)

But the idea of forcing the operand into a register for ColdFire was a
good one, and we shouldn't lose it.  The patch could be a performance
regression on ColdFire as-is.

We should either keep the force_reg, or restrict fp_src_operand to
TARGET_COLDFIRE_FPU.  I think the latter is easier because...

> -(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")))]
> +	(compare (match_operand:FP 0 "register_operand" "f,f")
> +		 (match_operand:FP 1 "fp_src_operand" "f,f<FP:dreg><Q>U")))]

...it would then work as-is here too.

Nitpick: the second "f" in operand 1's constraints is redundant.

> -{
> -  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";
> -})
> +  "@
> +   fcmp%.d %1,%0
> +   fcmp%.<FP:prec> %f1,%0")

You didn't say why you were getting rid of the CC_REVERSED stuff.
Are you sure it isn't useful?

Richard