[PATCH] AArch64 Neon Misc Scalar Arithmetic Instructions

[mcrosier at codeaurora.org]

Thanks, James.

Ping on the review, please.

 Chad

>>
>> 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
>> >>
>> >>
>> >
>>
>>
>>
>