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

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

    int foo (float f) { return 1.0 > f; }

which becomes:

    return f < 1.0e+0

So forcing constant first operands into a register is not as effective
as forcing constant second operands in a register.

The basic rule is: if the hardware insn needs a separate register load,
expose it as early as possible.  I thought that was the point of your
patch (and I agree it's a good idea).  I'm just trying to point out
a problem with the ColdFire side of things.

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

Yes, and because the ColdFire FPU's comparison insns don't allow
constant operands.  They're not as rich as the 68881 insns.

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

Well, AIUI, reload isn't smart enough to reverse comparisons.
Nor should it be.  It's already too much of a kitchen-sink pass.
I don't think we can really expect reload to avoid a temporary
register in this case _unless_ we allow operand 0 to be a MEM.

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

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

Richard