Re: [m68k 10/13] cleanup fp compare

[Richard Sandiford <richard at codesourcery dot com>]

Roman Zippel <zippel@linux-m68k.org> writes:
> 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.

Yeah, but the other operand. ;)  The operands aren't commutative after all.

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

Do you disagree that fp_src_operand should reject constants for
TARGET_COLDFIRE_FPU?

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

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

Richard