[PATCH, MIPS] Generate DMUL in widening multiplications

[Adam Nemet-3]

We discussed this in
<http://gcc.gnu.org/ml/gcc-patches/2009-10/msg00704.html>.

I centralized the decision of how to expand <u>mulsidi3 into a new function.
This also paves the way for the fast-dmult change.

Two points:

  * We could alternatively use enum insn_code (CODE_FOR_, etc.) rather than
    function pointers for the gen functions.

  * We could use mips_mulsidi3_gen_fn in the condition part of the underlying
    define_insns as well.  E.g.:

 (define_insn "<u>mulsidi3_64bit"
   [...]
-   "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
+   "mips_mulsidi3_gen_fn (<CODE> == SIGN_EXTEND) == gen_<u>mulsidi64_bit"

    The idea being that we always want to match the insn that we expanded.
    This not only looks a little funky but it's not actually true in case of
    the DSPr2 widening mult patterns.  So after pondering for a while I stayed
    with the existing practice.

Bootsrapped and regtested on mips64octeon-linux-gnu.  Also regtested on
mipsisa32-elf and mipsisa64-elf.

OK for mainline?

Adam


        * config/mips/mips-protos.h (mulsidi3_gen_fn): New typedef.
        (mips_mulsidi3_gen_fn): Declare new function.
        * config/mips/mips.c (mips_mulsidi3_gen_fn): New function.
        * config/mips/mips.md (<u>mulsidi3): Change condition to use
        mips_mulsidi3_gen_fn.  Use mips_mulsidi3_gen_fn to generate the
        insn.
        (<u>mulsidi3_64bit): Don't match for ISA_HAS_DMUL3.
        (mulsidi3_64bit_dmul): New define_insn.

testsuite/
        * gcc.target/mips/mult-1.c: Forbid octeon.
        * gcc.target/mips/octeon-dmul-3.c: New test.

Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc.orig/config/mips/mips-protos.h 2009-10-28 12:21:20.000000000 -0700
+++ gcc/config/mips/mips-protos.h 2009-10-28 12:21:44.000000000 -0700
@@ -345,4 +345,7 @@ extern bool mips_epilogue_uses (unsigned
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
 
+typedef rtx (*mulsidi3_gen_fn) (rtx, rtx, rtx);
+extern mulsidi3_gen_fn mips_mulsidi3_gen_fn (bool);
+
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc.orig/config/mips/mips.c 2009-10-28 12:21:20.000000000 -0700
+++ gcc/config/mips/mips.c 2009-10-29 10:43:22.000000000 -0700
@@ -15926,6 +15926,34 @@ mips_final_postscan_insn (FILE *file ATT
   if (mips_need_noat_wrapper_p (insn, opvec, noperands))
     mips_pop_asm_switch (&mips_noat);
 }
+
+/* Return the function that is used to expand the <u>mulsidi3 pattern.
+   SIGNED_ is true if the multiplication is signed.  Return NULL if widening
+   multiplication shouldn't be used.  */
+
+mulsidi3_gen_fn
+mips_mulsidi3_gen_fn (bool signed_)
+{
+  if (TARGET_64BIT)
+    {
+      /* Don't use widening multiplication with MULT when we have DMUL.  Even
+ with the extension of its input operands DMUL is faster.  Note that
+ the extension is not needed for signed multiplication.  In order to
+ ensure that we always remove the redundant sign-extension in this
+ case we still expand mulsidi3 for DMUL.  */
+      if (ISA_HAS_DMUL3)
+ return signed_ ? gen_mulsidi3_64bit_dmul : NULL;
+      if (TARGET_FIX_R4000)
+ return NULL;
+      return signed_ ? gen_mulsidi3_64bit : gen_umulsidi3_64bit;
+    }
+  else
+    {
+      if (TARGET_FIX_R4000)
+ return signed_ ? gen_mulsidi3_32bit_r4000 : gen_umulsidi3_32bit_r4000;
+      return signed_ ? gen_mulsidi3_32bit : gen_umulsidi3_32bit;
+    }
+}
 
 /* Return the size in bytes of the trampoline code, padded to
    TRAMPOLINE_ALIGNMENT bits.  The static chain pointer and target
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md 2009-10-28 12:21:20.000000000 -0700
+++ gcc/config/mips/mips.md 2009-10-28 14:38:41.000000000 -0700
@@ -1847,15 +1847,10 @@ (define_expand "<u>mulsidi3"
   [(set (match_operand:DI 0 "register_operand")
  (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand"))
  (any_extend:DI (match_operand:SI 2 "register_operand"))))]
-  "!TARGET_64BIT || !TARGET_FIX_R4000"
+  "mips_mulsidi3_gen_fn (<CODE> == SIGN_EXTEND) != NULL"
 {
-  if (TARGET_64BIT)
-    emit_insn (gen_<u>mulsidi3_64bit (operands[0], operands[1], operands[2]));
-  else if (TARGET_FIX_R4000)
-    emit_insn (gen_<u>mulsidi3_32bit_r4000 (operands[0], operands[1],
-    operands[2]));
-  else
-    emit_insn (gen_<u>mulsidi3_32bit (operands[0], operands[1], operands[2]));
+  mulsidi3_gen_fn fn = mips_mulsidi3_gen_fn (<CODE> == SIGN_EXTEND);
+  emit_insn (fn (operands[0], operands[1], operands[2]));
   DONE;
 })
 
@@ -1885,7 +1880,7 @@ (define_insn "<u>mulsidi3_64bit"
  (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
    (clobber (match_scratch:TI 3 "=x"))
    (clobber (match_scratch:DI 4 "=d"))]
-  "TARGET_64BIT && !TARGET_FIX_R4000"
+  "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
   "#"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")
@@ -1961,6 +1956,17 @@ (define_insn "<u>mulsidi3_64bit_hilo"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")])
 
+;; See comment before the ISA_HAS_DMUL3 case in mips_mulsidi3_gen_fn.
+(define_insn "mulsidi3_64bit_dmul"
+  [(set (match_operand:DI 0 "register_operand" "=d")
+ (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand" "d"))
+ (sign_extend:DI (match_operand:SI 2 "register_operand" "d"))))
+   (clobber (match_scratch:DI 3 "=l"))]
+  "TARGET_64BIT && ISA_HAS_DMUL3"
+  "dmul\t%0,%1,%2"
+  [(set_attr "type" "imul3")
+   (set_attr "mode" "DI")])
+
 ;; Widening multiply with negation.
 (define_insn "*muls<u>_di"
   [(set (match_operand:DI 0 "register_operand" "=x")
Index: gcc/testsuite/gcc.target/mips/mult-1.c
===================================================================
--- gcc.orig/testsuite/gcc.target/mips/mult-1.c 2009-10-28 12:21:20.000000000 -0700
+++ gcc/testsuite/gcc.target/mips/mult-1.c 2009-10-29 11:22:26.000000000 -0700
@@ -1,6 +1,6 @@
 /* For SI->DI widening multiplication we should use DINS to combine the two
-   halves.  */
-/* { dg-options "-O -mgp64 isa_rev>=2" } */
+   halves.  For Octeon use DMUL with explicit widening.  */
+/* { dg-options "-O -mgp64 isa_rev>=2 forbid_cpu=octeon" } */
 /* { dg-final { scan-assembler "\tdins\t" } } */
 /* { dg-final { scan-assembler-not "\tdsll\t" } } */
 /* { dg-final { scan-assembler-not "\tdsrl\t" } } */
Index: gcc/testsuite/gcc.target/mips/octeon-dmul-3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-dmul-3.c 2009-10-28 12:21:44.000000000 -0700
@@ -0,0 +1,19 @@
+/* Use DMUL for widening multiplication too.  */
+/* { dg-options "-O -march=octeon -mgp64" } */
+/* { dg-final { scan-assembler-times "\tdmul\t" 2 } } */
+/* { dg-final { scan-assembler-not "\td?mult\t" } } */
+/* { dg-final { scan-assembler-times "\tdext\t" 2 } } */
+
+NOMIPS16 long long
+f (int i, int j)
+{
+  i++;
+  return (long long) i * j;
+}
+
+NOMIPS16 unsigned long long
+g (unsigned int i, unsigned int j)
+{
+  i++;
+  return (unsigned long long) i * j;
+}

[Richard Sandiford-5]

Adam Nemet <anemet@...> writes:

>   * We could alternatively use enum insn_code (CODE_FOR_, etc.) rather than
>     function pointers for the gen functions.
>
>   * We could use mips_mulsidi3_gen_fn in the condition part of the underlying
>     define_insns as well.  E.g.:
>
>  (define_insn "<u>mulsidi3_64bit"
>    [...]
> -   "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
> +   "mips_mulsidi3_gen_fn (<CODE> == SIGN_EXTEND) == gen_<u>mulsidi64_bit"
>
>     The idea being that we always want to match the insn that we expanded.
>     This not only looks a little funky but it's not actually true in case of
>     the DSPr2 widening mult patterns.  So after pondering for a while I stayed
>     with the existing practice.

I don't think it's that bad, especially if you just pass the code,
not the result of the ==.  I'd rather do that than duplicate the insn
conditions (which could easily get out of sync otherwise).

Making the choice between the DSPr2 and non-DSPr2 patterns explicit
would be a good thing.

Also, the priority of the insns is handled more naturally with that
approach (fewer "&& !"s) and we're guaranteed to have mutually-
exclusive insn conditions.

Your alternative suggestion of using CODE_FOR_* would be neater for
these comparisons, but I suppose it sacrifices a bit of type-safety in
the define_expand code.  OTOH, the CODE_FOR_* enum leads you to whatever
info you need about an insn, whereas you can't really learn anything else
from a gen_* function.  Hmm.

I suppose there's no one good solution here, but I'm afraid I do prefer
both the alternatives you suggest.  I.e.

  - CODE_FOR_*s
  - using the new function in the define_insns too, and
  - passing the code directly, without the ==

Richard

[Adam Nemet-3]

Richard Sandiford writes:

> Adam Nemet <anemet@...> writes:
> >   * We could alternatively use enum insn_code (CODE_FOR_, etc.) rather than
> >     function pointers for the gen functions.
> >
> >   * We could use mips_mulsidi3_gen_fn in the condition part of the underlying
> >     define_insns as well.  E.g.:
> >
> >  (define_insn "<u>mulsidi3_64bit"
> >    [...]
> > -   "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
> > +   "mips_mulsidi3_gen_fn (<CODE> == SIGN_EXTEND) == gen_<u>mulsidi64_bit"
> >
> >     The idea being that we always want to match the insn that we expanded.
> >     This not only looks a little funky but it's not actually true in case of
> >     the DSPr2 widening mult patterns.  So after pondering for a while I stayed
> >     with the existing practice.
>
> I don't think it's that bad, especially if you just pass the code,
> not the result of the ==.  I'd rather do that than duplicate the insn
> conditions (which could easily get out of sync otherwise).
>
> Making the choice between the DSPr2 and non-DSPr2 patterns explicit
> would be a good thing.
>
> Also, the priority of the insns is handled more naturally with that
> approach (fewer "&& !"s) and we're guaranteed to have mutually-
> exclusive insn conditions.
>
> Your alternative suggestion of using CODE_FOR_* would be neater for
> these comparisons, but I suppose it sacrifices a bit of type-safety in
> the define_expand code.  OTOH, the CODE_FOR_* enum leads you to whatever
> info you need about an insn, whereas you can't really learn anything else
> from a gen_* function.  Hmm.
>
> I suppose there's no one good solution here, but I'm afraid I do prefer
> both the alternatives you suggest.  I.e.

No problem.  I was really on the fence about these :).

>
>   - CODE_FOR_*s
>   - using the new function in the define_insns too, and
>   - passing the code directly, without the ==

I had to limit the information in my email somewhere so I didn't mention one
additional problem with the combination of CODE_FOR and using
mips_mulsidi3_gen_fn in the condition of the define_insns.  I.e.:

(define_insn "<u>mulsidi3_64bit"
  [(set (match_operand:DI 0 "register_operand" "=d")
        (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
                 (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
   (clobber (match_scratch:TI 3 "=x"))
   (clobber (match_scratch:DI 4 "=d"))]
  "mips_mulsidi3_icode (<CODE>) == CODE_FOR_<u>mulsidi3_64bit"
  "#"

This fails to compile because gencondmd.c does not currently include
insn-codes.h.  In fact, very few modules include insn-codes.h.  If we want the
above construct we would have to change genconditions.c to also include
insn-codes.h into gencondmd.c.  Let me know if you still want to take this
route or use function pointers instead.

Adam

[Richard Sandiford-5]

Adam Nemet <anemet@...> writes:

> Richard Sandiford writes:
>>
>>   - CODE_FOR_*s
>>   - using the new function in the define_insns too, and
>>   - passing the code directly, without the ==
>
> I had to limit the information in my email somewhere so I didn't mention one
> additional problem with the combination of CODE_FOR and using
> mips_mulsidi3_gen_fn in the condition of the define_insns.  I.e.:
>
> (define_insn "<u>mulsidi3_64bit"
>   [(set (match_operand:DI 0 "register_operand" "=d")
> (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
> (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
>    (clobber (match_scratch:TI 3 "=x"))
>    (clobber (match_scratch:DI 4 "=d"))]
>   "mips_mulsidi3_icode (<CODE>) == CODE_FOR_<u>mulsidi3_64bit"
>   "#"
>
> This fails to compile because gencondmd.c does not currently include
> insn-codes.h.  In fact, very few modules include insn-codes.h.  If we want the
> above construct we would have to change genconditions.c to also include
> insn-codes.h into gencondmd.c.  Let me know if you still want to take this
> route or use function pointers instead.

I think it'd be cleaner, but that sort of target-independent change isn't
suitable at this stage.  OK, let's go with the function pointers then.

Richard

[Adam Nemet-3]

Richard Sandiford writes:

> Adam Nemet <anemet@...> writes:
> > Richard Sandiford writes:
> >>
> >>   - CODE_FOR_*s
> >>   - using the new function in the define_insns too, and
> >>   - passing the code directly, without the ==
> >
> > I had to limit the information in my email somewhere so I didn't mention one
> > additional problem with the combination of CODE_FOR and using
> > mips_mulsidi3_gen_fn in the condition of the define_insns.  I.e.:
> >
> > (define_insn "<u>mulsidi3_64bit"
> >   [(set (match_operand:DI 0 "register_operand" "=d")
> > (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
> > (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
> >    (clobber (match_scratch:TI 3 "=x"))
> >    (clobber (match_scratch:DI 4 "=d"))]
> >   "mips_mulsidi3_icode (<CODE>) == CODE_FOR_<u>mulsidi3_64bit"
> >   "#"
> >
> > This fails to compile because gencondmd.c does not currently include
> > insn-codes.h.  In fact, very few modules include insn-codes.h.  If we want the
> > above construct we would have to change genconditions.c to also include
> > insn-codes.h into gencondmd.c.  Let me know if you still want to take this
> > route or use function pointers instead.
>
> I think it'd be cleaner, but that sort of target-independent change isn't
> suitable at this stage.  OK, let's go with the function pointers then.

Sorry for misleading you, I could have sworn that I had tried this but that
does not work either.  While gencondmd.c includes tm.h which in turn includes
insn-flags.h which is where the gen_ functions are declared, insn-flags.h is
only included for non-GENERATOR_FILE compilations.  IOW, we can't use gen_
functions in conditions either:

   [(set (match_operand:DI 0 "register_operand" "=x")
  (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand" "d"))
  (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))]
-  "!TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DSPR2"
+  "mips_mulsidi3_gen_fn (<CODE>) == gen_<u>mulsidi3_32bit"
   "mult<u>\t%1,%2"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")])

:(.  So I don't think we can do better ATM than the current patch at:

  http://gcc.gnu.org/ml/gcc-patches/2009-10/msg01755.html

with the additional improvements of passing the rtx code to
mips_mulsidi3_gen_fn and also using the DSPr2 generators there.  Do you agree?

I am going to retest this the same way as before.  OK for mainline if it
passes?

Adam


        * config/mips/mips-protos.h (mulsidi3_gen_fn): New typedef.
        (mips_mulsidi3_gen_fn): Declare new function.
        * config/mips/mips.c (mips_mulsidi3_gen_fn): New function.
        * config/mips/mips.md (<u>mulsidi3): Change condition to use
        mips_mulsidi3_gen_fn.  Use mips_mulsidi3_gen_fn to generate the
        insn.
        (<u>mulsidi3_64bit): Don't match for ISA_HAS_DMUL3.
        (mulsidi3_64bit_dmul): New define_insn.

testsuite/
        * gcc.target/mips/mult-1.c: Forbid octeon.
        * gcc.target/mips/octeon-dmul-3.c: New test.

Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc.orig/config/mips/mips-protos.h 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips-protos.h 2009-11-12 10:53:27.000000000 -0800
@@ -345,4 +345,7 @@ extern bool mips_epilogue_uses (unsigned
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
 
+typedef rtx (*mulsidi3_gen_fn) (rtx, rtx, rtx);
+extern mulsidi3_gen_fn mips_mulsidi3_gen_fn (enum rtx_code);
+
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc.orig/config/mips/mips.c 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips.c 2009-11-12 10:56:45.000000000 -0800
@@ -15926,6 +15926,39 @@ mips_final_postscan_insn (FILE *file ATT
   if (mips_need_noat_wrapper_p (insn, opvec, noperands))
     mips_pop_asm_switch (&mips_noat);
 }
+
+/* Return the function that is used to expand the <u>mulsidi3 pattern.
+   EXT_CODE is the code of the extension used.  Return NULL if widening
+   multiplication shouldn't be used.  */
+
+mulsidi3_gen_fn
+mips_mulsidi3_gen_fn (enum rtx_code ext_code)
+{
+  bool signed_;
+
+  signed_ = ext_code == SIGN_EXTEND;
+  if (TARGET_64BIT)
+    {
+      /* Don't use widening multiplication with MULT when we have DMUL.  Even
+ with the extension of its input operands DMUL is faster.  Note that
+ the extension is not needed for signed multiplication.  In order to
+ ensure that we always remove the redundant sign-extension in this
+ case we still expand mulsidi3 for DMUL.  */
+      if (ISA_HAS_DMUL3)
+ return signed_ ? gen_mulsidi3_64bit_dmul : NULL;
+      if (TARGET_FIX_R4000)
+ return NULL;
+      return signed_ ? gen_mulsidi3_64bit : gen_umulsidi3_64bit;
+    }
+  else
+    {
+      if (TARGET_FIX_R4000)
+ return signed_ ? gen_mulsidi3_32bit_r4000 : gen_umulsidi3_32bit_r4000;
+      if (ISA_HAS_DSPR2)
+ return signed_ ? gen_mips_mult : gen_mips_multu;
+      return signed_ ? gen_mulsidi3_32bit : gen_umulsidi3_32bit;
+    }
+}
 
 /* Return the size in bytes of the trampoline code, padded to
    TRAMPOLINE_ALIGNMENT bits.  The static chain pointer and target
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips.md 2009-11-12 10:54:08.000000000 -0800
@@ -1847,15 +1847,10 @@ (define_expand "<u>mulsidi3"
   [(set (match_operand:DI 0 "register_operand")
  (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand"))
  (any_extend:DI (match_operand:SI 2 "register_operand"))))]
-  "!TARGET_64BIT || !TARGET_FIX_R4000"
+  "mips_mulsidi3_gen_fn (<CODE>) != NULL"
 {
-  if (TARGET_64BIT)
-    emit_insn (gen_<u>mulsidi3_64bit (operands[0], operands[1], operands[2]));
-  else if (TARGET_FIX_R4000)
-    emit_insn (gen_<u>mulsidi3_32bit_r4000 (operands[0], operands[1],
-    operands[2]));
-  else
-    emit_insn (gen_<u>mulsidi3_32bit (operands[0], operands[1], operands[2]));
+  mulsidi3_gen_fn fn = mips_mulsidi3_gen_fn (<CODE>);
+  emit_insn (fn (operands[0], operands[1], operands[2]));
   DONE;
 })
 
@@ -1885,7 +1880,7 @@ (define_insn "<u>mulsidi3_64bit"
  (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
    (clobber (match_scratch:TI 3 "=x"))
    (clobber (match_scratch:DI 4 "=d"))]
-  "TARGET_64BIT && !TARGET_FIX_R4000"
+  "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
   "#"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")
@@ -1961,6 +1956,17 @@ (define_insn "<u>mulsidi3_64bit_hilo"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")])
 
+;; See comment before the ISA_HAS_DMUL3 case in mips_mulsidi3_gen_fn.
+(define_insn "mulsidi3_64bit_dmul"
+  [(set (match_operand:DI 0 "register_operand" "=d")
+ (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand" "d"))
+ (sign_extend:DI (match_operand:SI 2 "register_operand" "d"))))
+   (clobber (match_scratch:DI 3 "=l"))]
+  "TARGET_64BIT && ISA_HAS_DMUL3"
+  "dmul\t%0,%1,%2"
+  [(set_attr "type" "imul3")
+   (set_attr "mode" "DI")])
+
 ;; Widening multiply with negation.
 (define_insn "*muls<u>_di"
   [(set (match_operand:DI 0 "register_operand" "=x")
Index: gcc/testsuite/gcc.target/mips/mult-1.c
===================================================================
--- gcc.orig/testsuite/gcc.target/mips/mult-1.c 2009-11-12 10:52:00.000000000 -0800
+++ gcc/testsuite/gcc.target/mips/mult-1.c 2009-11-12 10:52:29.000000000 -0800
@@ -1,6 +1,6 @@
 /* For SI->DI widening multiplication we should use DINS to combine the two
-   halves.  */
-/* { dg-options "-O -mgp64 isa_rev>=2" } */
+   halves.  For Octeon use DMUL with explicit widening.  */
+/* { dg-options "-O -mgp64 isa_rev>=2 forbid_cpu=octeon" } */
 /* { dg-final { scan-assembler "\tdins\t" } } */
 /* { dg-final { scan-assembler-not "\tdsll\t" } } */
 /* { dg-final { scan-assembler-not "\tdsrl\t" } } */
Index: gcc/testsuite/gcc.target/mips/octeon-dmul-3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-dmul-3.c 2009-11-12 10:52:29.000000000 -0800
@@ -0,0 +1,19 @@
+/* Use DMUL for widening multiplication too.  */
+/* { dg-options "-O -march=octeon -mgp64" } */
+/* { dg-final { scan-assembler-times "\tdmul\t" 2 } } */
+/* { dg-final { scan-assembler-not "\td?mult\t" } } */
+/* { dg-final { scan-assembler-times "\tdext\t" 2 } } */
+
+NOMIPS16 long long
+f (int i, int j)
+{
+  i++;
+  return (long long) i * j;
+}
+
+NOMIPS16 unsigned long long
+g (unsigned int i, unsigned int j)
+{
+  i++;
+  return (unsigned long long) i * j;
+}

[Adam Nemet-3]

Adam Nemet <anemet@...> writes:
> :(.  So I don't think we can do better ATM than the current patch at:
>
>   http://gcc.gnu.org/ml/gcc-patches/2009-10/msg01755.html
>
> with the additional improvements of passing the rtx code to
> mips_mulsidi3_gen_fn and also using the DSPr2 generators there.  Do you agree?
>
> I am going to retest this the same way as before.  OK for mainline if it
> passes?

There was an #ifdef RTX_CODE needed in mips-protos.h.

Bootstrapped and regtested on mips64octeon-linux-gnu.  Regtested on
mipsisa32-elf and mipsisa64-elf.

OK for mainline?

Adam


        * config/mips/mips-protos.h (mulsidi3_gen_fn): New typedef.
        (mips_mulsidi3_gen_fn): Declare new function.
        * config/mips/mips.c (mips_mulsidi3_gen_fn): New function.
        * config/mips/mips.md (<u>mulsidi3): Change condition to use
        mips_mulsidi3_gen_fn.  Use mips_mulsidi3_gen_fn to generate the
        insn.
        (<u>mulsidi3_64bit): Don't match for ISA_HAS_DMUL3.
        (mulsidi3_64bit_dmul): New define_insn.

testsuite/
        * gcc.target/mips/mult-1.c: Forbid octeon.
        * gcc.target/mips/octeon-dmul-3.c: New test.

Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc.orig/config/mips/mips-protos.h 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips-protos.h 2009-11-13 09:25:09.000000000 -0800
@@ -345,4 +345,9 @@ extern bool mips_epilogue_uses (unsigned
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
 
+typedef rtx (*mulsidi3_gen_fn) (rtx, rtx, rtx);
+#ifdef RTX_CODE
+extern mulsidi3_gen_fn mips_mulsidi3_gen_fn (enum rtx_code);
+#endif
+
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc.orig/config/mips/mips.c 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips.c 2009-11-12 10:56:45.000000000 -0800
@@ -15926,6 +15926,39 @@ mips_final_postscan_insn (FILE *file ATT
   if (mips_need_noat_wrapper_p (insn, opvec, noperands))
     mips_pop_asm_switch (&mips_noat);
 }
+
+/* Return the function that is used to expand the <u>mulsidi3 pattern.
+   EXT_CODE is the code of the extension used.  Return NULL if widening
+   multiplication shouldn't be used.  */
+
+mulsidi3_gen_fn
+mips_mulsidi3_gen_fn (enum rtx_code ext_code)
+{
+  bool signed_;
+
+  signed_ = ext_code == SIGN_EXTEND;
+  if (TARGET_64BIT)
+    {
+      /* Don't use widening multiplication with MULT when we have DMUL.  Even
+ with the extension of its input operands DMUL is faster.  Note that
+ the extension is not needed for signed multiplication.  In order to
+ ensure that we always remove the redundant sign-extension in this
+ case we still expand mulsidi3 for DMUL.  */
+      if (ISA_HAS_DMUL3)
+ return signed_ ? gen_mulsidi3_64bit_dmul : NULL;
+      if (TARGET_FIX_R4000)
+ return NULL;
+      return signed_ ? gen_mulsidi3_64bit : gen_umulsidi3_64bit;
+    }
+  else
+    {
+      if (TARGET_FIX_R4000)
+ return signed_ ? gen_mulsidi3_32bit_r4000 : gen_umulsidi3_32bit_r4000;
+      if (ISA_HAS_DSPR2)
+ return signed_ ? gen_mips_mult : gen_mips_multu;
+      return signed_ ? gen_mulsidi3_32bit : gen_umulsidi3_32bit;
+    }
+}
 
 /* Return the size in bytes of the trampoline code, padded to
    TRAMPOLINE_ALIGNMENT bits.  The static chain pointer and target
Index: gcc/config/mips/mips.md
===================================================================
--- gcc.orig/config/mips/mips.md 2009-11-12 10:52:00.000000000 -0800
+++ gcc/config/mips/mips.md 2009-11-12 10:54:08.000000000 -0800
@@ -1847,15 +1847,10 @@ (define_expand "<u>mulsidi3"
   [(set (match_operand:DI 0 "register_operand")
  (mult:DI (any_extend:DI (match_operand:SI 1 "register_operand"))
  (any_extend:DI (match_operand:SI 2 "register_operand"))))]
-  "!TARGET_64BIT || !TARGET_FIX_R4000"
+  "mips_mulsidi3_gen_fn (<CODE>) != NULL"
 {
-  if (TARGET_64BIT)
-    emit_insn (gen_<u>mulsidi3_64bit (operands[0], operands[1], operands[2]));
-  else if (TARGET_FIX_R4000)
-    emit_insn (gen_<u>mulsidi3_32bit_r4000 (operands[0], operands[1],
-    operands[2]));
-  else
-    emit_insn (gen_<u>mulsidi3_32bit (operands[0], operands[1], operands[2]));
+  mulsidi3_gen_fn fn = mips_mulsidi3_gen_fn (<CODE>);
+  emit_insn (fn (operands[0], operands[1], operands[2]));
   DONE;
 })
 
@@ -1885,7 +1880,7 @@ (define_insn "<u>mulsidi3_64bit"
  (any_extend:DI (match_operand:SI 2 "register_operand" "d"))))
    (clobber (match_scratch:TI 3 "=x"))
    (clobber (match_scratch:DI 4 "=d"))]
-  "TARGET_64BIT && !TARGET_FIX_R4000"
+  "TARGET_64BIT && !TARGET_FIX_R4000 && !ISA_HAS_DMUL3"
   "#"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")
@@ -1961,6 +1956,17 @@ (define_insn "<u>mulsidi3_64bit_hilo"
   [(set_attr "type" "imul")
    (set_attr "mode" "SI")])
 
+;; See comment before the ISA_HAS_DMUL3 case in mips_mulsidi3_gen_fn.
+(define_insn "mulsidi3_64bit_dmul"
+  [(set (match_operand:DI 0 "register_operand" "=d")
+ (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand" "d"))
+ (sign_extend:DI (match_operand:SI 2 "register_operand" "d"))))
+   (clobber (match_scratch:DI 3 "=l"))]
+  "TARGET_64BIT && ISA_HAS_DMUL3"
+  "dmul\t%0,%1,%2"
+  [(set_attr "type" "imul3")
+   (set_attr "mode" "DI")])
+
 ;; Widening multiply with negation.
 (define_insn "*muls<u>_di"
   [(set (match_operand:DI 0 "register_operand" "=x")
Index: gcc/testsuite/gcc.target/mips/mult-1.c
===================================================================
--- gcc.orig/testsuite/gcc.target/mips/mult-1.c 2009-11-12 10:52:00.000000000 -0800
+++ gcc/testsuite/gcc.target/mips/mult-1.c 2009-11-12 10:52:29.000000000 -0800
@@ -1,6 +1,6 @@
 /* For SI->DI widening multiplication we should use DINS to combine the two
-   halves.  */
-/* { dg-options "-O -mgp64 isa_rev>=2" } */
+   halves.  For Octeon use DMUL with explicit widening.  */
+/* { dg-options "-O -mgp64 isa_rev>=2 forbid_cpu=octeon" } */
 /* { dg-final { scan-assembler "\tdins\t" } } */
 /* { dg-final { scan-assembler-not "\tdsll\t" } } */
 /* { dg-final { scan-assembler-not "\tdsrl\t" } } */
Index: gcc/testsuite/gcc.target/mips/octeon-dmul-3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.target/mips/octeon-dmul-3.c 2009-11-12 10:52:29.000000000 -0800
@@ -0,0 +1,19 @@
+/* Use DMUL for widening multiplication too.  */
+/* { dg-options "-O -march=octeon -mgp64" } */
+/* { dg-final { scan-assembler-times "\tdmul\t" 2 } } */
+/* { dg-final { scan-assembler-not "\td?mult\t" } } */
+/* { dg-final { scan-assembler-times "\tdext\t" 2 } } */
+
+NOMIPS16 long long
+f (int i, int j)
+{
+  i++;
+  return (long long) i * j;
+}
+
+NOMIPS16 unsigned long long
+g (unsigned int i, unsigned int j)
+{
+  i++;
+  return (unsigned long long) i * j;
+}

[Richard Sandiford-5]

Adam Nemet <anemet@...> writes:
> Adam Nemet <anemet@...> writes:
>> :(.  So I don't think we can do better ATM than the current patch at:
>>
>>   http://gcc.gnu.org/ml/gcc-patches/2009-10/msg01755.html
>>
>> with the additional improvements of passing the rtx code to
>> mips_mulsidi3_gen_fn and also using the DSPr2 generators there.  Do you agree?

Well, I suppose.  This isn't what we'd do during stage 1, but since you
gave me a heads-up during stage 1, and submitted the patch a few weeks ago,
let's go with it for 4.5.

> +  bool signed_;

signed_p

OK with that change, thanks.

Richard