<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">That seems very reasonable, but I'm rather new to this territory so I<br>
don't have a definitive answer.  However, I don't see any cases in the<br>AArchInstrNEON.td file that does this, so for consistency sake I assume it<br>would be fine to leave it as a space.</blockquote><div><br></div>
<div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-family:arial,sans-serif;font-size:13px">To answer your questions, the Neon_Scalar3Same_BHSD_size_</span><span style="font-family:arial,sans-serif;font-size:13px">patterns<br>
</span><span style="font-family:arial,sans-serif;font-size:13px">multiclass inherits from the Neon_Scalar3Same_D_size_</span><span style="font-family:arial,sans-serif;font-size:13px">patterns multiclass.<br></span><span style="font-family:arial,sans-serif;font-size:13px">Thus, Neon_Scalar3Same_D_size_</span><span style="font-family:arial,sans-serif;font-size:13px">patterns must be a multiclass.</span></blockquote>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I see, thanks! I hadn't spotted that.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-family:arial,sans-serif;font-size:13px">Ana and I are reusing the int_arm_* intrinsics when possible.  If the<br>
</span><span style="font-family:arial,sans-serif;font-size:13px">instruction is completely new with ARMv8, then we're creating a new<br></span><span style="font-family:arial,sans-serif;font-size:13px">intrinsic.</span></blockquote>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">That makes sense.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">So it LGTM, but I don't know enough to properly review, so please wait for someone else to chime in.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Cheers,</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">James</span></div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On 3 October 2013 18:14,  <span dir="ltr"><<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi James,<br>
Thanks for the quick feedback.<br>
<div class="im"><br>
> Hi Chad,<br>
><br>
> Just after having a very quick once-over:<br>
><br>
> +                                !strconcat(asmop, " $Rd, $Rn, $Rm"),<br>
><br>
> Is it not normal to separate the opcode and operands with '\t' instead of<br>
> '<br>
> '?<br>
<br>
</div>That seems very reasonable, but I'm rather new to this territory so I<br>
don't have a definitive answer.  However, I don't see any cases in the<br>
AArchInstrNEON.td file that does this, so for consistency sake I assume it<br>
would be fine to leave it as a space.<br>
<div class="im"><br>
><br>
> -multiclass Neon_Scalar_D_size_patterns<SDPatternOperator opnode,<br>
> -                                       Instruction INSTD> {<br>
> +multiclass Neon_Scalar3Same_D_size_patterns<SDPatternOperator opnode,<br>
> +                                            Instruction INSTD> {<br>
>    def : Pat<(v1i64 (opnode (v1i64 FPR64:$Rn), (v1i64 FPR64:$Rm))),<br>
>              (INSTD FPR64:$Rn, FPR64:$Rm)>;<br>
>  }<br>
><br>
> Is there any particular reason you're adding a multiclass with only one<br>
> (anonymous) element? Is it just so the later definitions can all<br>
> consistently be "defm" or are you going to be adding to this class?<br>
<br>
</div>My change was just to rename Neon_Scalar_D_size_patterns to<br>
Neon_Scalar3Same_D_size_patterns for consistency sake.<br>
<br>
To answer your questions, the Neon_Scalar3Same_BHSD_size_patterns<br>
multiclass inherits from the Neon_Scalar3Same_D_size_patterns multiclass.<br>
Thus, Neon_Scalar3Same_D_size_patterns must be a multiclass.<br>
<div class="im"><br>
> -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqadds, SQADDddd>;<br>
> -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqaddu, UQADDddd>;<br>
> -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqsubs, SQSUBddd>;<br>
> -defm : Neon_Scalar_D_size_patterns<int_arm_neon_vqsubu, UQSUBddd>;<br>
> +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqadds, SQADDddd>;<br>
> +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqaddu, UQADDddd>;<br>
> +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqsubs, SQSUBddd>;<br>
> +defm : Neon_Scalar3Same_D_size_patterns<int_arm_neon_vqsubu, UQSUBddd>;<br>
><br>
> I'm sure this has been mentioned before, and you've obviously not changed<br>
> this, but are we deliberately reusing all v7/AArch32 intrinsics<br>
> ("int_arm_*") and only adding new ones under "int_aarch64_*"? or is this<br>
> code an oversight?<br>
<br>
</div>Ana and I are reusing the int_arm_* intrinsics when possible.  If the<br>
instruction is completely new with ARMv8, then we're creating a new<br>
intrinsic.<br>
<div class="im"><br>
> Apologies for any stupidity, I haven't been following along much with the<br>
> v8 NEON implementation.<br>
><br>
<br>
</div>No worries.  Thanks for the feedback, James!!  Hopefully, Tim, Joey, Jim<br>
or someone else can have a look.<br>
<br>
 Chad<br>
<div class="HOEnZb"><div class="h5"><br>
> Cheers,<br>
><br>
> James<br>
><br>
><br>
> On 3 October 2013 17:39, <<a href="mailto:mcrosier@codeaurora.org">mcrosier@codeaurora.org</a>> wrote:<br>
><br>
>> All,<br>
>> The attached patches contain clang front-end and LLVM back-end code<br>
>> changes to add support for the following NEON scalar arithmetic<br>
>> instructions:<br>
>><br>
>> -Signed saturating doubling multiply high half - SQDMULH<br>
>> -Signed saturating rounding doubling multiply high half - SQRDMULH<br>
>> -Floating-point multiply extended - FMULX<br>
>> -Floating-point reciprocal step - FRECPS<br>
>> -Floating-point reciprocal square root step - FRSQRTS<br>
>><br>
>> Please have a look!<br>
>><br>
>>  Chad<br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>><br>
>><br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div>