This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, MIPS] Generate DMUL in widening multiplications


Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]