[PATCH] AArch64 Neon Misc Scalar Arithmetic Instructions

[James Molloy]

>
> That seems very reasonable, but I'm rather new to this territory so I
> don't have a definitive answer.  However, I don't see any cases in the
> AArchInstrNEON.td file that does this, so for consistency sake I assume it
> would be fine to leave it as a space.


Sure. If all the other instructions use space it makes sense to keep it
that way. But as all the instructions in the ARM backend (and I'd assume
all the AArch64 scalar insts although I haven't checked) use tab, we should
probably do a pass through at some point and make them all use the same
thing.

To answer your questions, the Neon_Scalar3Same_BHSD_size_patterns
> multiclass inherits from the Neon_Scalar3Same_D_size_patterns multiclass.
> Thus, Neon_Scalar3Same_D_size_patterns must be a multiclass.


I see, thanks! I hadn't spotted that.

Ana and I are reusing the int_arm_* intrinsics when possible.  If the
> instruction is completely new with ARMv8, then we're creating a new
> intrinsic.


That makes sense.

So it LGTM, but I don't know enough to properly review, so please wait for
someone else to chime in.

Cheers,

James


On 3 October 2013 18:14, <mcrosier at codeaurora.org> wrote:

> Hi James,
> Thanks for the quick feedback.
>
> > Hi Chad,
> >
> > Just after having a very quick once-over:
> >
> > +                                !strconcat(asmop, " $Rd, $Rn, $Rm"),
> >
> > Is it not normal to separate the opcode and operands with '\t' instead of
> > '
> > '?
>
> That seems very reasonable, but I'm rather new to this territory so I
> don't have a definitive answer.  However, I don't see any cases in the
> AArchInstrNEON.td file that does this, so for consistency sake I assume it
> would be fine to leave it as a space.
>
> >
> > -multiclass Neon_Scalar_D_size_patterns<SDPatternOperator opnode,
> > -                                       Instruction INSTD> {
> > +multiclass Neon_Scalar3Same_D_size_patterns<SDPatternOperator opnode,
> > +                                            Instruction INSTD> {
> >    def : Pat<(v1i64 (opnode (v1i64 FPR64:$Rn), (v1i64 FPR64:$Rm))),
> >              (INSTD FPR64:$Rn, FPR64:$Rm)>;
> >  }
> >
> > Is there any particular reason you're adding a multiclass with only one
> > (anonymous) element? Is it just so the later definitions can all
> > consistently be "defm" or are you going to be adding to this class?
>
> My change was just to rename Neon_Scalar_D_size_patterns to
> Neon_Scalar3Same_D_size_patterns for consistency sake.
>
> To answer your questions, the Neon_Scalar3Same_BHSD_size_patterns
> multiclass inherits from the Neon_Scalar3Same_D_size_patterns multiclass.
> Thus, Neon_Scalar3Same_D_size_patterns must be a multiclass.
>
> > -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqadds, SQADDddd>;
> > -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqaddu, UQADDddd>;
> > -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqsubs, SQSUBddd>;
> > -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqsubu, UQSUBddd>;
> > +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqadds, SQADDddd>;
> > +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqaddu, UQADDddd>;
> > +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqsubs, SQSUBddd>;
> > +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqsubu, UQSUBddd>;
> >
> > I'm sure this has been mentioned before, and you've obviously not changed
> > this, but are we deliberately reusing all v7/AArch32 intrinsics
> > ("int_arm_*") and only adding new ones under "int_aarch64_*"? or is this
> > code an oversight?
>
> Ana and I are reusing the int_arm_* intrinsics when possible.  If the
> instruction is completely new with ARMv8, then we're creating a new
> intrinsic.
>
> > Apologies for any stupidity, I haven't been following along much with the
> > v8 NEON implementation.
> >
>
> No worries.  Thanks for the feedback, James!!  Hopefully, Tim, Joey, Jim
> or someone else can have a look.
>
>  Chad
>
> > Cheers,
> >
> > James
> >
> >
> > On 3 October 2013 17:39, <mcrosier at codeaurora.org> wrote:
> >
> >> All,
> >> The attached patches contain clang front-end and LLVM back-end code
> >> changes to add support for the following NEON scalar arithmetic
> >> instructions:
> >>
> >> -Signed saturating doubling multiply high half - SQDMULH
> >> -Signed saturating rounding doubling multiply high half - SQRDMULH
> >> -Floating-point multiply extended - FMULX
> >> -Floating-point reciprocal step - FRECPS
> >> -Floating-point reciprocal square root step - FRSQRTS
> >>
> >> Please have a look!
> >>
> >>  Chad
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >>
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131004/e812b479/attachment.html>