[m68k 10/13] cleanup fp compare

[Roman Zippel]

Hi,

On Mon, 5 Feb 2007, Richard Sandiford wrote:

> >> 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.
> 
> Yeah, but the other operand. ;)  The operands aren't commutative after all.

What difference does it make at this point?

> > I don't think this will cause a perfomance regression, as combine later 
> > had all the freedom to put the value back.
> 
> But it would only do that if the compare instruction was the only user.
> combine wouldn't have got rid of the copy if there were _two_ users.

AFAIK [g]cse should have put the value into a register in this case.

> >> 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.
> 
> Well, like I say, the old code was a half-measure.  (It was presumably
> better than nothing though, otherwise it would never have been added.)
> What we really want is to force one operand to be a nonimmediate_operand
> and one to be a register_operand.  As I say, the best way to do that
> 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?

> >> > -  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.
> 
> But why was it a problem?  Consider the case where we have:
> 
>     (set (cc0)
>          (compare (reg:SF PSEUDO1)
>                   (reg:SF PSEUDO2)))
> 
> before reload.  PSEUDO2 gets allocated a hard register H and PSEUDO1
> gets spilled to the stack at address A.  The old pattern allowed:
> 
>     (set (cc0)
>          (compare (mem:SF A)
>                   (reg:SF H)))
> 
> keeping the comparison at one instruction.  Yours forces reload to
> move (mem:SF A) into a temporary register T first, which is both an
> extra instruction, and has the possiblity of kicking out whatever
> the register allocators put into T.

I'm trying to avoid unnecessary reloads by putting one of the values 
already into a register before reload. In your case reload already has too 
few registers and has to do reloads anyway and can still make another 
try at reducing reloads some other way. This makes it a relative special 
case, where I'm trying to do something for the general case.
I wouldn't mind to help reload for this case, but I'd prefer if it were 
possible without losing the more general improvements.

bye, Roman