thr3ads.net - opus - [opus] [PATCH] Optimize silk_LPC_inverse_pred

If this information is useful, please help other people find it:
Share via:

Linfeng Zhang

2017-Feb-15 01:18 UTC

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

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>

Jean-Marc Valin

2017-Feb-15 07:35 UTC

head link

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

Hi Linfeng,

Thanks for the updated patch. Just pushed it to master. One thing that
still bothers me a bit is that the
if( ( max > 0 ) || ( min < -1 ) )
line is still pretty much untested. By which I mean that if I remove the
condition, then the tests (including the new unit tests) still pass. I
wasn't able to figure out a case that triggers it -- and I'm not even
100% sure it's actually possible. It would be nice if you could look
into that, but I didn't think it was worth holding up your patch over that.

Cheers,

	Jean-Marc

On 14/02/17 08:18 PM, Linfeng Zhang wrote:> 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
> <mailto: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>
>     > <mailto: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>
<mailto:opus at xiph.org
>     <mailto:opus at xiph.org>>
>     >     > http://lists.xiph.org/mailman/listinfo/opus
>     <http://lists.xiph.org/mailman/listinfo/opus>
>     >     >
>     >
> 
>

Linfeng Zhang

2017-Feb-15 18:17 UTC

head link

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

Hi Jean-Marc,

Thanks for reviewing and merging this patch!

For "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.

If we cut

    max_s32x2 = vmax_s32( vget_low_s32( max_s32x4 ), vget_high_s32(
max_s32x4 ) );
    min_s32x2 = vmin_s32( vget_low_s32( min_s32x4 ), vget_high_s32(
min_s32x4 ) );
    max_s32x2 = vmax_s32( max_s32x2, vreinterpret_s32_s64( vshr_n_s64(
vreinterpret_s64_s32( max_s32x2 ), 32 ) ) );
    min_s32x2 = vmin_s32( min_s32x2, vreinterpret_s32_s64( vshr_n_s64(
vreinterpret_s64_s32( min_s32x2 ), 32 ) ) );
    max = vget_lane_s32( max_s32x2, 0 );
    min = vget_lane_s32( min_s32x2, 0 );
    if( ( max > 0 ) || ( min < -1 ) ) {
        return 0;
    }

and paste into the inner for loop at the position between
            min_s32x4   = vminq_s32( min_s32x4, s1_s32x4 );
and
            t1_s32x4    = vrev64q_s32( t1_s32x4 );
then it will be triggered, and the optimization is still correct.

Thanks,
Linfeng


On Tue, Feb 14, 2017 at 11:35 PM, Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> Hi Linfeng,
>
> Thanks for the updated patch. Just pushed it to master. One thing that
> still bothers me a bit is that the
> if( ( max > 0 ) || ( min < -1 ) )
> line is still pretty much untested. By which I mean that if I remove the
> condition, then the tests (including the new unit tests) still pass. I
> wasn't able to figure out a case that triggers it -- and I'm not
even
> 100% sure it's actually possible. It would be nice if you could look
> into that, but I didn't think it was worth holding up your patch over
that.
>
> Cheers,
>
>         Jean-Marc
>
> On 14/02/17 08:18 PM, Linfeng Zhang wrote:
> > 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
> > <mailto: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>
> >     > <mailto: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>
<mailto:opus at xiph.org
> >     <mailto:opus at xiph.org>>
> >     >     > http://lists.xiph.org/mailman/listinfo/opus
> >     <http://lists.xiph.org/mailman/listinfo/opus>
> >     >     >
> >     >
> >
> >
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170215/5452e702/attachment.html>

Apparently Analagous Threads

Search for more seemingly similar threads

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

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

[opus] [PATCH] Optimize silk_LPC_inverse_pred_gain() for ARM NEON

Apparently Analagous Threads