<div dir="ltr">Hi Jean-Marc,<div><br></div><div>Thanks for reviewing and merging this patch!</div><div><br></div><div>For "<span style="font-size:12.8px">if( ( max > 0 ) || ( min < -1 ) )" not triggering, the reason is that we postpone the testing until the big for loop is done for efficiency purpose. Then its return was caught by the earlier returns in the loop.</span></div><div><br></div><div>If we cut </div><div><br></div><div><div> max_s32x2 = vmax_s32( vget_low_s32( max_s32x4 ), vget_high_s32( max_s32x4 ) );</div><div> min_s32x2 = vmin_s32( vget_low_s32( min_s32x4 ), vget_high_s32( min_s32x4 ) );</div><div> max_s32x2 = vmax_s32( max_s32x2, vreinterpret_s32_s64( vshr_n_s64( vreinterpret_s64_s32( max_s32x2 ), 32 ) ) );</div><div> min_s32x2 = vmin_s32( min_s32x2, vreinterpret_s32_s64( vshr_n_s64( vreinterpret_s64_s32( min_s32x2 ), 32 ) ) );</div><div> max = vget_lane_s32( max_s32x2, 0 );</div><div> min = vget_lane_s32( min_s32x2, 0 );</div><div> if( ( max > 0 ) || ( min < -1 ) ) {</div><div> return 0;</div><div> }</div></div><div><br></div><div>and paste into the inner for loop at the position between</div><div> min_s32x4 = vminq_s32( min_s32x4, s1_s32x4 );<br></div><div>and<br></div><div><div> t1_s32x4 = vrev64q_s32( t1_s32x4 );</div></div><div>then it will be triggered, and the optimization is still correct.</div><div><br></div><div>Thanks,</div><div>Linfeng</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 14, 2017 at 11:35 PM, Jean-Marc Valin <span dir="ltr"><<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Linfeng,<br>
<br>
Thanks for the updated patch. Just pushed it to master. One thing that<br>
still bothers me a bit is that the<br>
if( ( max > 0 ) || ( min < -1 ) )<br>
line is still pretty much untested. By which I mean that if I remove the<br>
condition, then the tests (including the new unit tests) still pass. I<br>
wasn't able to figure out a case that triggers it -- and I'm not even<br>
100% sure it's actually possible. It would be nice if you could look<br>
into that, but I didn't think it was worth holding up your patch over that.<br>
<br>
Cheers,<br>
<br>
Jean-Marc<br>
<span class=""><br>
On 14/02/17 08:18 PM, Linfeng Zhang wrote:<br>
> Hi Jean-Marc, (forgot cc opus@)<br>
><br>
> Thanks for creating the unit test code.<br>
><br>
> Attached is the updated optimization patch.<br>
><br>
> On Mon, Feb 13, 2017 at 10:17 AM, Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a><br>
</span><span class="">> <mailto:<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a>>> wrote:<br>
><br>
> On 13/02/17 01:09 PM, Linfeng Zhang wrote:<br>
> > For 1), I agree that an explicit unit test would be a good plus to cover<br>
> > the cases that "make check" cannot trigger. If you like, we may submit<br>
> > an unit test patch for code review.<br>
><br>
> Yes, please include a unit test that triggers the overflow detection.<br>
> Once that works, I think we can merge this optimization.<br>
><br>
> Cheers,<br>
><br>
> Jean-Marc<br>
><br>
> > Thanks,<br>
> > Linfeng<br>
> ><br>
> > On Thu, Feb 9, 2017 at 4:48 PM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a> <mailto:<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a>><br>
</span><div><div class="h5">> > <mailto:<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a> <mailto:<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a>>>> wrote:<br>
> ><br>
> > Hi Linfeng,<br>
> ><br>
> > Can you confirm that you the patch went through the same<br>
> internal review<br>
> > (presumably from James) than the previous ones?<br>
> ><br>
> > I had a look and did some testing and it looked good to me.<br>
> There's only<br>
> > two issues I'd like to resolve first -- none of which directly<br>
> related<br>
> > to your code.<br>
> ><br>
> > 1) The overflow condition is essentially untested because none<br>
> of the<br>
> > tests in "make check" reliably triggers it. That may be a good<br>
> case for<br>
> > an explicit unit test. IIRC, the case could be triggered by the<br>
> > following input vector:<br>
> > A_QA[] = { 46596096, -72118272, 78532608, -69447680, 52707328,<br>
> > -22073344, -19890176, 50507776, -54829056, 45518848, -33939456,<br>
> > 21086208, -7127040, -4136960, 3993600, -1699840 }<br>
> ><br>
> > 2) I'm not quite sure what to make of<br>
> silk_LPC_inverse_pred_gain_<wbr>Q24().<br>
> > It seems to never be called anywhere -- except from the MIPS<br>
> code. Maybe<br>
> > it should just stay as it is (not renamed to _c()) but I need<br>
> to check.<br>
> > Any thoughts?<br>
> ><br>
> > Cheers,<br>
> ><br>
> > Jean-Marc<br>
> ><br>
> > On 07/02/17 04:06 PM, Linfeng Zhang wrote:<br>
> > > Hi,<br>
> > ><br>
> > > Attached is a patch with arm neon optimizations for<br>
> > > silk_LPC_inverse_pred_gain(). Please review.<br>
> > ><br>
> > > Thanks,<br>
> > > Linfeng<br>
> > ><br>
> > ><br>
> > ><br>
> > > ______________________________<wbr>_________________<br>
> > > opus mailing list<br>
</div></div>> > > <a href="mailto:opus@xiph.org">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a><br>
> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>>><br>
> > > <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/<wbr>listinfo/opus</a><br>
> <<a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/<wbr>mailman/listinfo/opus</a>><br>
> > ><br>
> ><br>
><br>
><br>
</blockquote></div><br></div>