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

[Linfeng Zhang]

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>