Re: [m68k 10/13] cleanup fp compare

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

Hi,

On Mon, 5 Feb 2007, Richard Sandiford wrote:

> 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.

One of the operands is still forced into a register.
I don't think this will cause a perfomance regression, as combine later 
had all the freedom to put the value back.

> 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.

Why do you want to force _both_ values into a register, the old code 
didn't do this and I don't see a reason why it would be any advantage.

> > -  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?

Because it's done in combine now. With the old pattern the register 
operand could be reloaded into any of the two operands, which isn't a 
problem with the new pattern anymore.

bye, Roman