Workarounds for issues with rs6000_rtx_costs

[Roger Sayle <roger at eyesopen dot com>]

Hi David and Geoff,

I just wanted to check with you that its OK for me to commit my
"combine_insn_cost" patch to mainline CVS?  The patch has been
approved by Richard Henderson, but as I've mentioned in a
previous posting this will regress gcc.dg/ppc-fmadd-1.c on
powerpc*-*-*.  The issue isn't an problem with the patch itself,
nor causes incorrect code to be generated, but it may cause
poorly parameterized backends, or latent bugs in TARGET_RTX_COSTS,
to generate inferior code.

I've investigated the failure of pcc-fmadd-1 on powerpc-apple-darwin7.4
and the problem is caused by the patch now refusing to combine the
following instructions:

(insn 50 49 51 0 (set (reg:DF 147)
        (plus:DF (mult:DF (reg:DF 143)
                (reg:DF 144))
            (reg:DF 146))) 204 {*rs6000.md:4912} (insn_list 47 (insn_list
46 (insn_list 49 (nil))))
    (expr_list:REG_DEAD (reg:DF 144)
        (expr_list:REG_DEAD (reg:DF 143)
            (expr_list:REG_DEAD (reg:DF 146)
                (nil)))))

(insn 51 50 52 0 (set (reg:DF 148)
        (neg:DF (reg:DF 147))) 197 {negdf2} (insn_list 50 (nil))
    (expr_list:REG_DEAD (reg:DF 147)
        (nil)))


which would previously be combined into the following instruction:

(insn 51 50 52 0 (set (reg:DF 148)
        (minus:DF (mult:DF (neg:DF (reg:DF 143))
                (reg:DF 144))
            (reg:DF 146))) 207 {*rs6000.md:4940} (insn_list 49 (insn_list
46 (insn_list 47 (nil))))
    (expr_list:REG_DEAD (reg:DF 146)
        (expr_list:REG_DEAD (reg:DF 143)
            (expr_list:REG_DEAD (reg:DF 144)
                (nil)))))


The issue is that the rs6000's backends report the rtx_cost of the
original two instructions as each COSTS_N_INSNS (1), but that the
replacement instruction reports itself as COSTS_N_INSNS (6).  Hence
this latest patch rejects this merge as 24 is larger than 8.

The underlying cause are some bugs in rs6000.c's rs6000_rtx_costs;
a large discrepancy between the way that PLUS and MINUS are handled.

The code for case PLUS: doesn't check its operands, and instead
increments the total by COSTS_N_INSNS (1) and returns true.  Returning
true causes rtx_cost not to sum the costs of the addition's operands.
Hence, the low cost of the original insn 50 above.  For MINUS,
however, there's currently no explicit case, so it's handled by
the middle-end's default behaviour.  Which is to cost COST_N_INSNS (1)
*plus* the cost of its operands.  Clearly, including the multiplication
(and negation?) makes the MINUS form of the fmadd significantly more
expense the the PLUS form.

To resolve the ppc-fmadd-1.c regression there are two possible
workarounds (hacks).  The first is have PLUS return false, so that
it includes the cost of the addition's operands.


Index: rs6000.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
retrieving revision 1.655
diff -c -3 -p -r1.655 rs6000.c
*** rs6000.c    29 Jun 2004 19:27:05 -0000      1.655
--- rs6000.c    29 Jun 2004 19:51:53 -0000
*************** rs6000_rtx_costs (rtx x, int code, int o
*** 16167,16173 ****
                 && ((INTVAL (XEXP (x, 1)) & 0xffff) != 0))
                ? COSTS_N_INSNS (2)
                : COSTS_N_INSNS (1));
!       return true;

      case AND:
      case IOR:
--- 16167,16173 ----
                 && ((INTVAL (XEXP (x, 1)) & 0xffff) != 0))
                ? COSTS_N_INSNS (2)
                : COSTS_N_INSNS (1));
!       return false;

      case AND:
      case IOR:

The second is to handle MINUS in exactly the same way as we currently
handle PLUS.

Index: rs6000.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/rs6000/rs6000.c,v
retrieving revision 1.655
diff -c -3 -p -r1.655 rs6000.c
*** rs6000.c    29 Jun 2004 19:27:05 -0000      1.655
--- rs6000.c    29 Jun 2004 19:54:23 -0000
*************** rs6000_rtx_costs (rtx x, int code, int o
*** 16169,16174 ****
--- 16169,16178 ----
                : COSTS_N_INSNS (1));
        return true;

+     case MINUS:
+       *total = COSTS_N_INSNS (1);
+       return true;
+
      case AND:
      case IOR:
      case XOR:


Obviously, both of these fixes are just workarounds for the correct
solution, to provide floating point (and optimize_size?) costs in
the rs6000 backend.  Obviously, a floating point fused multiply add
instruction should be more expensive than a single cycle integer
addition.  I'd be happy to work on a patch to transition the rs6000
backend to a structure-based processor_cost implementation, like that
used by the x86 backend, but clearly this approach isn't an immediate
solution.


Is it OK with you guys if I can go ahead an commit my patch to
mainline, and that we address gcc.dg/ppc-fmadd-1.c as a follow-up
issue?  I'm a bit uncomfortable committing a patch to CVS that's
known to cause testsuite failures, without at least clearing it
with the affected back-end maintainers.  At the very least, Geoff's
regression checker will scream at me with an hour or two. :>


Many thanks in advance, and sorry for any inconvenience,

Roger
--