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][GCC][AARCH64] Canonicalize aarch64 widening simd plus insns



On 24/07/18 16:12, James Greenhalgh wrote:
On Thu, Jul 19, 2018 at 07:35:22AM -0500, Matthew Malcomson wrote:
> Hi again.
>
> Providing an updated patch to include the formatting suggestions.

Please try not to top-post replies, it makes the conversation thread
harder to follow (reply continues below!).

> On 12/07/18 11:39, Sudakshina Das wrote:
> > Hi Matthew
> >
> > On 12/07/18 11:18, Richard Sandiford wrote:
> >> Looks good to me FWIW (not a maintainer), just a minor formatting thing:
> >>
> >> Matthew Malcomson <matthew.malcomson@arm.com> writes:
> >>> diff --git a/gcc/config/aarch64/aarch64-simd.md
> >>> b/gcc/config/aarch64/aarch64-simd.md
> >>> index
> >>> aac5fa146ed8dde4507a0eb4ad6a07ce78d2f0cd..67b29cbe2cad91e031ee23be656ec61a403f2cf9
> >>> 100644
> >>> --- a/gcc/config/aarch64/aarch64-simd.md
> >>> +++ b/gcc/config/aarch64/aarch64-simd.md
> >>> @@ -3302,38 +3302,78 @@
> >>>     DONE;
> >>>   })
> >>>   -(define_insn "aarch64_<ANY_EXTEND:su><ADDSUB:optab>w<mode>"
> >>> +(define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> >>>     [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> >>> -        (ADDSUB:<VWIDE> (match_operand:<VWIDE> 1 "register_operand"
> >>> "w")
> >>> -            (ANY_EXTEND:<VWIDE>
> >>> -              (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>> +            (minus:<VWIDE>
> >>> +             (match_operand:<VWIDE> 1 "register_operand" "w")
> >>> +             (ANY_EXTEND:<VWIDE>
> >>> +               (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>
> >> The (minus should be under the "(match_operand":
> >>
> >> (define_insn "aarch64_<ANY_EXTEND:su>subw<mode>"
> >>    [(set (match_operand:<VWIDE> 0 "register_operand" "=w")
> >>     (minus:<VWIDE> (match_operand:<VWIDE> 1 "register_operand" "w")
> >>                (ANY_EXTEND:<VWIDE>
> >>              (match_operand:VD_BHSI 2 "register_operand" "w"))))]
> >>
> >> Same for the other patterns.
> >>
> >> Thanks,
> >> Richard
> >>
> >
> > You will need a maintainer's approval but this looks good to me.
> > Thanks for doing this. I would only point out one other nit which you
> > can choose to ignore:
> >
> > +/* Ensure
> > +   saddw2 and one saddw for the function add()
> > +   ssubw2 and one ssubw for the function subtract()
> > +   uaddw2 and one uaddw for the function uadd()
> > +   usubw2 and one usubw for the function usubtract() */
> > +
> > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]ssubw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]saddw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]saddw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]usubw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]usubw\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw2\[ \t\]+" 1 } } */
> > +/* { dg-final { scan-assembler-times "\[ \t\]uaddw\[ \t\]+" 1 } } */
> >
> > The scan-assembly directives for the different
> > functions can be placed right below each of them and that would
> > make it easier to read the expected results in the test and you
> > can get rid of the comments saying the same.

Thanks for the first-line review Sudi.

OK for trunk.


I've committed this on behalf of Matthew as: https://gcc.gnu.org/r262949

Thanks,
Kyrill

Thanks,
James



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