opus - Feb 2017 - [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

Hi Jean-Marc,

Yes I confirm that we have done the same internal review on this patch.

For 1), I agree that an explicit unit test would be a good plus to cover
the cases that "make check" cannot trigger. If you like, we may submit
an
unit test patch for code review.

Thanks,
Linfeng

On Thu, Feb 9, 2017 at 4:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> Hi Linfeng,
>
> Can you confirm that you the patch went through the same internal review
> (presumably from James) than the previous ones?
>
> I had a look and did some testing and it looked good to me. There's
only
> two issues I'd like to resolve first -- none of which directly related
> to your code.
>
> 1) The overflow condition is essentially untested because none of the
> tests in "make check" reliably triggers it. That may be a good
case for
> an explicit unit test. IIRC, the case could be triggered by the
> following input vector:
> A_QA[] = { 46596096, -72118272, 78532608, -69447680, 52707328,
> -22073344, -19890176, 50507776, -54829056, 45518848, -33939456,
> 21086208, -7127040, -4136960, 3993600, -1699840 }
>
> 2) I'm not quite sure what to make of silk_LPC_inverse_pred_gain_Q24().
> It seems to never be called anywhere -- except from the MIPS code. Maybe
> it should just stay as it is (not renamed to _c()) but I need to check.
> Any thoughts?
>
> Cheers,
>
>         Jean-Marc
>
> On 07/02/17 04:06 PM, Linfeng Zhang wrote:
> > Hi,
> >
> > Attached is a patch with arm neon optimizations for
> > silk_LPC_inverse_pred_gain(). Please review.
> >
> > Thanks,
> > Linfeng
> >
> >
> >
> > _______________________________________________
> > opus mailing list
> > opus at xiph.org
> > http://lists.xiph.org/mailman/listinfo/opus
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170213/1ae6979d/attachment.html>

On 13/02/17 01:09 PM, Linfeng Zhang wrote:
> For 1), I agree that an explicit unit test would be a good plus to cover
> the cases that "make check" cannot trigger. If you like, we may
submit
> an unit test patch for code review.
Yes, please include a unit test that triggers the overflow detection.
Once that works, I think we can merge this optimization.

Cheers,

	Jean-Marc
> Thanks,
> Linfeng
> 
> On Thu, Feb 9, 2017 at 4:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
> 
>     Hi Linfeng,
> 
>     Can you confirm that you the patch went through the same internal
review
>     (presumably from James) than the previous ones?
> 
>     I had a look and did some testing and it looked good to me. There's
only
>     two issues I'd like to resolve first -- none of which directly
related
>     to your code.
> 
>     1) The overflow condition is essentially untested because none of the
>     tests in "make check" reliably triggers it. That may be a
good case for
>     an explicit unit test. IIRC, the case could be triggered by the
>     following input vector:
>     A_QA[] = { 46596096, -72118272, 78532608, -69447680, 52707328,
>     -22073344, -19890176, 50507776, -54829056, 45518848, -33939456,
>     21086208, -7127040, -4136960, 3993600, -1699840 }
> 
>     2) I'm not quite sure what to make of
silk_LPC_inverse_pred_gain_Q24().
>     It seems to never be called anywhere -- except from the MIPS code.
Maybe
>     it should just stay as it is (not renamed to _c()) but I need to check.
>     Any thoughts?
> 
>     Cheers,
> 
>             Jean-Marc
> 
>     On 07/02/17 04:06 PM, Linfeng Zhang wrote:
>     > Hi,
>     >
>     > Attached is a patch with arm neon optimizations for
>     > silk_LPC_inverse_pred_gain(). Please review.
>     >
>     > Thanks,
>     > Linfeng
>     >
>     >
>     >
>     > _______________________________________________
>     > opus mailing list
>     > opus at xiph.org <mailto:opus at xiph.org>
>     > http://lists.xiph.org/mailman/listinfo/opus
>     >
>

Hi Jean-Marc, (forgot cc opus@)

Thanks for creating the unit test code.

Attached is the updated optimization patch.

On Mon, Feb 13, 2017 at 10:17 AM, Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> On 13/02/17 01:09 PM, Linfeng Zhang wrote:
> > For 1), I agree that an explicit unit test would be a good plus to
cover
> > the cases that "make check" cannot trigger. If you like, we
may submit
> > an unit test patch for code review.
>
> Yes, please include a unit test that triggers the overflow detection.
> Once that works, I think we can merge this optimization.
>
> Cheers,
>
>         Jean-Marc
>
> > Thanks,
> > Linfeng
> >
> > On Thu, Feb 9, 2017 at 4:48 PM Jean-Marc Valin <jmvalin at
jmvalin.ca
> > <mailto:jmvalin at jmvalin.ca>> wrote:
> >
> >     Hi Linfeng,
> >
> >     Can you confirm that you the patch went through the same internal
> review
> >     (presumably from James) than the previous ones?
> >
> >     I had a look and did some testing and it looked good to me.
There's
> only
> >     two issues I'd like to resolve first -- none of which directly
> related
> >     to your code.
> >
> >     1) The overflow condition is essentially untested because none of
the
> >     tests in "make check" reliably triggers it. That may be
a good case
> for
> >     an explicit unit test. IIRC, the case could be triggered by the
> >     following input vector:
> >     A_QA[] = { 46596096, -72118272, 78532608, -69447680, 52707328,
> >     -22073344, -19890176, 50507776, -54829056, 45518848, -33939456,
> >     21086208, -7127040, -4136960, 3993600, -1699840 }
> >
> >     2) I'm not quite sure what to make of
silk_LPC_inverse_pred_gain_
> Q24().
> >     It seems to never be called anywhere -- except from the MIPS code.
> Maybe
> >     it should just stay as it is (not renamed to _c()) but I need to
> check.
> >     Any thoughts?
> >
> >     Cheers,
> >
> >             Jean-Marc
> >
> >     On 07/02/17 04:06 PM, Linfeng Zhang wrote:
> >     > Hi,
> >     >
> >     > Attached is a patch with arm neon optimizations for
> >     > silk_LPC_inverse_pred_gain(). Please review.
> >     >
> >     > Thanks,
> >     > Linfeng
> >     >
> >     >
> >     >
> >     > _______________________________________________
> >     > opus mailing list
> >     > opus at xiph.org <mailto:opus at xiph.org>
> >     > http://lists.xiph.org/mailman/listinfo/opus
> >     >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170214/15f19f2c/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Optimize-silk_LPC_inverse_pred_gain-for-ARM-NEON.patch
Type: text/x-patch
Size: 36466 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170214/15f19f2c/attachment-0001.bin>