[PATCH 3/4] [ARC] Refurbish mul64 support.
Claudiu Zissulescu
Claudiu.Zissulescu@synopsys.com
Fri Dec 16 13:25:00 GMT 2016
Committed with the suggested mods.
Thank you for ur review,
Claudiu
> -----Original Message-----
> From: Andrew Burgess [mailto:andrew.burgess@embecosm.com]
> Sent: Wednesday, December 14, 2016 2:01 PM
> To: Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com>
> Cc: gcc-patches@gcc.gnu.org; Francois.Bedard@synopsys.com
> Subject: Re: [PATCH 3/4] [ARC] Refurbish mul64 support.
>
> * Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-11-16
> 11:18:00 +0100]:
>
> > gcc/
> > 2016-07-04 Claudiu Zissulescu <claziss@synopsys.com>
> >
> > * config/arc/arc.md (mulsidi_600): Changed.
> > (umulsidi_600): Likewise.
> > (mul64): New pattern.
> > (mulu64): Likewise.
> > (mulsidi3): Changed.
> > (umulsidi3): Likewise.
>
> As always the changelog description is pretty limited. Remember, it's
> a _change_ log, if you mention something I already know it's changed,
> better to say what has changed. Here's what I'd write for this patch:
>
> * config/arc/arc.md (mulsidi_600): Change to insn_and_split,
> generate new mul64 insn for core multiplication work.
> (umulsidi_600): Likewise, but generate mulu64 insn.
> (mul64): New insn pattern, content largely taken from old
> mulsidi_600 insn pattern.
> (mulu64): Likewise, but content from old umulsidi_600
> pattern.
> (mulsidi3): Remove move to destination, this is now handled by
> mulsidi_600 insn_and_split.
> (umulsidi3): Likewise, but using new umulsidi_600.
>
> It never hurts (I think) to include a longer description of the change
> with the commit, especially when the commit should produce no user
> visible change in the final result and is just a clean up. This has
> two benefits I think, first, if we do ever track a bug back to this
> commit (and the commit log say that no user visible changes are
> expected) then we know, with 100% certainty, that this was a mistake
> and not just an unfortunate side effect of an intended change.
> Second, stating that no user visible changes are expected explains why
> there are no tests with the patch; the assumption would be that the
> functionality is already covered by existing tests.
>
> Here's what I'd write for this commit:
>
> Rework 64-bit multiplication patterns
>
> Previously users of mulsidi_600 and umulsidi_600 had to take
> care of moving the multiplication result into the final
> destination themselves (from the MUL64_OUT_REG register).
> This commit converts these two instruction patterns into
> insn_and_split patterns that now take the final destination as
> an extra operand. The insn_and_split patterns generate the
> multiplication using two new multiplication instruction
> patterns, then generate the move of the result from the
> MUL64_OUT_REG register into the final destination.
>
> This is a clean up commit, there should be no user visible
> changes after this commit.
>
> I tend to prefer move verbose commit logs, which are not to everyones
> taste, but I'm sure there's some middle ground we can find between my
> ultra-verbose style and your ultra-brief style :)
>
> But otherwise, this looks fine.
>
> Thanks,
> Andrew
>
>
> > ---
> > gcc/config/arc/arc.md | 64 ++++++++++++++++++++++++++++++++-------
> ------------
> > 1 file changed, 40 insertions(+), 24 deletions(-)
> >
> > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> > index 86de423..76a3207 100644
> > --- a/gcc/config/arc/arc.md
> > +++ b/gcc/config/arc/arc.md
> > @@ -12,10 +12,6 @@
> > ;; Profiling support and performance improvements by
> > ;; Joern Rennecke (joern.rennecke@embecosm.com)
> > ;;
> > -;; Support for DSP multiply instructions and mul64
> > -;; instructions for ARC600; and improvements in flag setting
> > -;; instructions by
> > -;; Muhammad Khurram Riaz (Khurram.Riaz@arc.com)
> >
> > ;; This file is part of GCC.
> >
> > @@ -2011,14 +2007,26 @@
> > [(set_attr "is_sfunc" "yes")
> > (set_attr "predicable" "yes")])
> >
> > -(define_insn "mulsidi_600"
> > +(define_insn_and_split "mulsidi_600"
> > + [(set (match_operand:DI 0 "register_operand" "=c, c,c,
> c")
> > + (mult:DI (sign_extend:DI (match_operand:SI 1 "register_operand"
> "%Rcq#q, c,c, c"))
> > + (sign_extend:DI (match_operand:SI 2
> "nonmemory_operand" "Rcq#q,cL,L,C32"))))
> > + (clobber (reg:DI MUL64_OUT_REG))]
> > + "TARGET_MUL64_SET"
> > + "#"
> > + "TARGET_MUL64_SET"
> > + [(const_int 0)]
> > + "emit_insn (gen_mul64 (operands[1], operands[2]));
> > + emit_move_insn (operands[0], gen_rtx_REG (DImode,
> MUL64_OUT_REG));
> > + DONE;"
> > + [(set_attr "type" "multi")
> > + (set_attr "length" "8")])
> > +
> > +(define_insn "mul64"
> > [(set (reg:DI MUL64_OUT_REG)
> > - (mult:DI (sign_extend:DI
> > - (match_operand:SI 0 "register_operand" "%Rcq#q,c,c,c"))
> > - (sign_extend:DI
> > -; assembler issue for "I", see mulsi_600
> > -; (match_operand:SI 1 "register_operand"
> "Rcq#q,cL,I,Cal"))))]
> > - (match_operand:SI 1 "register_operand"
> "Rcq#q,cL,L,C32"))))]
> > + (mult:DI
> > + (sign_extend:DI (match_operand:SI 0 "register_operand" "%Rcq#q,
> c,c, c"))
> > + (sign_extend:DI (match_operand:SI 1 "nonmemory_operand"
> "Rcq#q,cL,L,C32"))))]
> > "TARGET_MUL64_SET"
> > "mul64%? \t0, %0, %1%&"
> > [(set_attr "length" "*,4,4,8")
> > @@ -2027,14 +2035,26 @@
> > (set_attr "predicable" "yes,yes,no,yes")
> > (set_attr "cond" "canuse,canuse,canuse_limm,canuse")])
> >
> > -(define_insn "umulsidi_600"
> > +(define_insn_and_split "umulsidi_600"
> > + [(set (match_operand:DI 0 "register_operand" "=c,c, c")
> > + (mult:DI (zero_extend:DI (match_operand:SI 1 "register_operand"
> "%c,c, c"))
> > + (sign_extend:DI (match_operand:SI 2
> "nonmemory_operand" "cL,L,C32"))))
> > + (clobber (reg:DI MUL64_OUT_REG))]
> > + "TARGET_MUL64_SET"
> > + "#"
> > + "TARGET_MUL64_SET"
> > + [(const_int 0)]
> > + "emit_insn (gen_mulu64 (operands[1], operands[2]));
> > + emit_move_insn (operands[0], gen_rtx_REG (DImode,
> MUL64_OUT_REG));
> > + DONE;"
> > + [(set_attr "type" "umulti")
> > + (set_attr "length" "8")])
> > +
> > +(define_insn "mulu64"
> > [(set (reg:DI MUL64_OUT_REG)
> > - (mult:DI (zero_extend:DI
> > - (match_operand:SI 0 "register_operand" "%c,c,c"))
> > - (sign_extend:DI
> > -; assembler issue for "I", see mulsi_600
> > -; (match_operand:SI 1 "register_operand" "cL,I,Cal"))))]
> > - (match_operand:SI 1 "register_operand" "cL,L,C32"))))]
> > + (mult:DI
> > + (zero_extend:DI (match_operand:SI 0 "register_operand" "%c,c,c"))
> > + (zero_extend:DI (match_operand:SI 1 "nonmemory_operand"
> "cL,L,C32"))))]
> > "TARGET_MUL64_SET"
> > "mulu64%? \t0, %0, %1%&"
> > [(set_attr "length" "4,4,8")
> > @@ -2098,9 +2118,7 @@
> > }
> > else if (TARGET_MUL64_SET)
> > {
> > - operands[2] = force_reg (SImode, operands[2]);
> > - emit_insn (gen_mulsidi_600 (operands[1], operands[2]));
> > - emit_move_insn (operands[0], gen_rtx_REG (DImode,
> MUL64_OUT_REG));
> > + emit_insn (gen_mulsidi_600 (operands[0], operands[1], operands[2]));
> > DONE;
> > }
> > else if (TARGET_MULMAC_32BY16_SET)
> > @@ -2332,9 +2350,7 @@
> > }
> > else if (TARGET_MUL64_SET)
> > {
> > - operands[2] = force_reg (SImode, operands[2]);
> > - emit_insn (gen_umulsidi_600 (operands[1], operands[2]));
> > - emit_move_insn (operands[0], gen_rtx_REG (DImode,
> MUL64_OUT_REG));
> > + emit_insn (gen_umulsidi_600 (operands[0], operands[1],
> operands[2]));
> > DONE;
> > }
> > else if (TARGET_MULMAC_32BY16_SET)
> > --
> > 1.9.1
> >
More information about the Gcc-patches
mailing list