[opus] [PATCH] fix alignment exceptions

[Ray Essick]

Jonathan,
Here's the code difference we see with the recent change -- what amounts to
reverting your change from a couple years back.
It doesn't look like we're getting superfluous instructions from clang now.

the bad behavior for us was the alignment exception on the movdqa
instructions when the input data wasn't 128-bit aligned.
We had to change something because the code as-is was taking alignment
faults on the movdqa instructions.

For reference, the clang version  I used for this is:
    | Android clang version 5.0.300080  (based on LLVM 5.0.300080)
    | Target: x86_64-unknown-linux


If we think enough people use older versions of clang, a version of the
patch that looked at __clang_major__ and friends seems fair.

-- Ray

826% diff -c *old *new
*** pitch_sse4_1.s-old 2017-08-18 13:51:39.359084637 -0700
--- pitch_sse4_1.s-new 2017-08-18 13:51:54.595106450 -0700
***************
*** 73,80 ****
  cmpl $4, %eax
  jl .LBB0_8
  # BB#7:
! movdqa (%edx,%edi,2), %xmm2
! movdqa (%esi,%edi,2), %xmm1
  addl $4, %edi
  movdqa %xmm2, %xmm3
  pmullw %xmm1, %xmm2
--- 73,80 ----
  cmpl $4, %eax
  jl .LBB0_8
  # BB#7:
! movq (%edx,%edi,2), %xmm2    # xmm2 = mem[0],zero
! movq (%esi,%edi,2), %xmm1    # xmm1 = mem[0],zero
  addl $4, %edi
  movdqa %xmm2, %xmm3
  pmullw %xmm1, %xmm2


On Fri, Aug 18, 2017 at 12:11 PM, Felicia Lim <flim at google.com> wrote:

> We see the MOVQ instruction but this patch deliberately uses it rather
> than MOVQDA (load 128-bits aligned). We were seeing that with the trace
> below, the final invocation is not 128-bit aligned but MOVQDA insists on it
> (the calling function was pitch_sse4_1.c:90, in the 4-way N - i >= 4
> loop).
>
> 07-31 11:00:13.469   210  2540 <(469)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3deb0 y 0xeff3deb0 N 32
> 07-31 11:00:13.469   210  2540 <(469)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3d7b0 y 0xeff3d7b0 N 32
> 07-31 11:00:13.469   210  2540 <(469)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3df30 y 0xeff3df30 N 32
> 07-31 11:00:13.470   210  2540 <(470)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3d850 y 0xeff3d850 N 48
> 07-31 11:00:13.470   210  2540 <(470)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3dfd0 y 0xeff3dfd0 N 48
> 07-31 11:00:13.470   210  2540 <(470)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3d8b0 y 0xeff3d8b0 N 64
> 07-31 11:00:13.470   210  2540 <(470)%20210-2540> D opus_sse1: RBE
> celt_inner_prod_sse4_1: x 0xeff3e030 y 0xeff3e030 N 64
> 07-31 11:00:13.476   210  2540 D opus_sse1: RBE celt_inner_prod_sse4_1: x
> 0xeff3da38 y 0xeff3da38 N 36
>
>
> On Fri, Aug 18, 2017 at 11:44 AM Jonathan Lennox <jonathan at vidyo.com>
> wrote:
>
>> This would revert a patch I submitted two years ago (
>> http://git.xiph.org/?p=opus.git;a=commitdiff;h=
>> 1d60b49e9d95672a17ebe5578319c59fa3963224).
>>
>> At the time, clang produced an unnecessary MOVQ instruction when it
>> compiled with the version of the code with the explicit _mm_loadl_epi64
>> intrinsic.  Do you no longer see that?
>>
>> How does the code compile for you, and what is the issue you’re seeing?
>>  (One issue might be that clang’s address sanitizer isn’t smart enough to
>> know that PMOVSXWD only loads 8 bytes, despite _mm_cvtepi16_epi32’s
>> argument being an __mm128i; I’ve seen it trigger incorrect out-of-bounds
>> read errors.)
>>
>> On Aug 18, 2017, at 12:34 PM, Felicia Lim <flim at google.com> wrote:
>>
>> Hi,
>>
>> Please find attached a patch to fix alignment exceptions. Without this
>> change, we were seeing occasional alignment faults when using this with
>> clang.
>>
>> Thanks,
>> Felicia
>>
>> <e5c277c.diff>_______________________________________________
>> 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/20170818/f7384b44/attachment-0001.html>