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

Hi,

Attached is a patch with arm neon optimizations for
silk_LPC_inverse_pred_gain(). Please review.

Thanks,
Linfeng
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170207/5c5ab508/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: 36790 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170207/5c5ab508/attachment-0001.bin>

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
>

On 09/02/17 07:48 PM, Jean-Marc Valin wrote:
> 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?
OK, turns out the last use of that function was removed between 1.1.x
and current master. I just checked in a change that removes
silk_LPC_inverse_pred_gain_Q24() completely.

Cheers,

	Jean-Marc

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>