<div dir="ltr">Thanks, this has been merged in master.</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Aug 22, 2017 at 4:34 PM Jonathan Lennox <<a href="mailto:jonathan@vidyo.com">jonathan@vidyo.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">



<div style="word-wrap:break-word">
<div>Hm, yes â€” with the current Xcode clang (Xcode 8.3.3, Apple LLVM version 8.1.0 (clang-802.0.42)), I don’t see any difference in the generated code with -O2 between the two versions. Â (Literally none for x86_64, and only what seem to be some non-signficiant
 offset changes for i386).</div>
<div><br>
</div>
<div>So if this improves some other cases (notably, it should avoid problems with -fsanitize=address) I think this is okay.</div></div><div style="word-wrap:break-word">
<br>
<div>
<blockquote type="cite">
<div>On Aug 18, 2017, at 5:25 PM, Ray Essick <<a href="mailto:essick@google.com" target="_blank">essick@google.com</a>> wrote:</div>
<br class="m_4806609671514967972Apple-interchange-newline">
<div>
<div dir="ltr">Jonathan,
<div>Here's the code difference we see with the recent change -- what amounts to reverting your change from a couple years back.</div>
<div>It doesn't look like we're getting superfluous instructions from clang now.</div>
<div><br>
</div>
<div>the bad behavior for us was the alignment exception on the movdqa instructions when the input data wasn't 128-bit aligned.</div>
<div>We had to change something because the code as-is was taking alignment faults on the movdqa instructions.</div>
<div><br>
</div>
<div>
<div>For reference, the clang version Â I used for this is:</div>
<div>  Â  | Android clang version 5.0.300080 Â (based on LLVM 5.0.300080)<br>
</div>
<div>  Â  | Target: x86_64-unknown-linux</div>
</div>
<div><br>
</div>
<div><br>
</div>
<div>If we think enough people use older versions of clang, a version of the patch that looked at __clang_major__ and friends seems fair.</div>
<div><br>
</div>
<div>-- Ray</div>
<div><br>
</div>
<div>
<div>826% diff -c *old *new</div>
<div>*** pitch_sse4_1.s-old<span style="white-space:pre-wrap"> </span>
2017-08-18 13:51:39.359084637 -0700</div>
<div>--- pitch_sse4_1.s-new<span style="white-space:pre-wrap"> </span>
2017-08-18 13:51:54.595106450 -0700</div>
<div>***************</div>
<div>*** 73,80 ****</div>
<div>  <span style="white-space:pre-wrap"></span>cmpl<span style="white-space:pre-wrap">
</span>$4, %eax</div>
<div>  <span style="white-space:pre-wrap"></span>jl<span style="white-space:pre-wrap">
</span>.LBB0_8</div>
<div>  # BB#7:</div>
<div>! <span style="white-space:pre-wrap"></span>movdqa<span style="white-space:pre-wrap">
</span>(%edx,%edi,2), %xmm2</div>
<div>! <span style="white-space:pre-wrap"></span>movdqa<span style="white-space:pre-wrap">
</span>(%esi,%edi,2), %xmm1</div>
<div>  <span style="white-space:pre-wrap"></span>addl<span style="white-space:pre-wrap">
</span>$4, %edi</div>
<div>  <span style="white-space:pre-wrap"></span>movdqa<span style="white-space:pre-wrap">
</span>%xmm2, %xmm3</div>
<div>  <span style="white-space:pre-wrap"></span>pmullw<span style="white-space:pre-wrap">
</span>%xmm1, %xmm2</div>
<div>--- 73,80 ----</div>
<div>  <span style="white-space:pre-wrap"></span>cmpl<span style="white-space:pre-wrap">
</span>$4, %eax</div>
<div>  <span style="white-space:pre-wrap"></span>jl<span style="white-space:pre-wrap">
</span>.LBB0_8</div>
<div>  # BB#7:</div>
<div>! <span style="white-space:pre-wrap"></span>movq<span style="white-space:pre-wrap">
</span>(%edx,%edi,2), %xmm2 Â  Â # xmm2 = mem[0],zero</div>
<div>! <span style="white-space:pre-wrap"></span>movq<span style="white-space:pre-wrap">
</span>(%esi,%edi,2), %xmm1 Â  Â # xmm1 = mem[0],zero</div>
<div>  <span style="white-space:pre-wrap"></span>addl<span style="white-space:pre-wrap">
</span>$4, %edi</div>
<div>  <span style="white-space:pre-wrap"></span>movdqa<span style="white-space:pre-wrap">
</span>%xmm2, %xmm3</div>
<div>  <span style="white-space:pre-wrap"></span>pmullw<span style="white-space:pre-wrap">
</span>%xmm1, %xmm2</div>
</div>
<div><br>
</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Aug 18, 2017 at 12:11 PM, Felicia Lim <span dir="ltr">
<<a href="mailto:flim@google.com" target="_blank">flim@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div>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 <span style="font-family:monospace;font-size:12px;white-space:pre-wrap">N - i >= 4
</span>loop).</div>
<div><br>
</div>
<div><span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(469)%20210-2540" value="+14692102540" target="_blank">469 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3deb0
 y 0xeff3deb0 N 32</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(469)%20210-2540" value="+14692102540" target="_blank">469 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3d7b0 y 0xeff3d7b0
 N 32</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(469)%20210-2540" value="+14692102540" target="_blank">469 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3df30 y 0xeff3df30
 N 32</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(470)%20210-2540" value="+14702102540" target="_blank">470 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3d850 y 0xeff3d850
 N 48</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(470)%20210-2540" value="+14702102540" target="_blank">470 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3dfd0 y 0xeff3dfd0
 N 48</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(470)%20210-2540" value="+14702102540" target="_blank">470 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3d8b0 y 0xeff3d8b0
 N 64</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.<a href="tel:(470)%20210-2540" value="+14702102540" target="_blank">470 Â  210 Â 2540</a> D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3e030 y 0xeff3e030
 N 64</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
<span style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">07-31 11:00:13.476 Â  210 Â 2540 D opus_sse1: RBE celt_inner_prod_sse4_1: x 0xeff3da38 y 0xeff3da38 N 36</span><br style="color:rgb(34,34,34);font-family:Arial,Helvetica,sans-serif">
</div>
<div dir="ltr">
<div><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Aug 18, 2017 at 11:44 AM Jonathan Lennox <<a href="mailto:jonathan@vidyo.com" target="_blank">jonathan@vidyo.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">
<div>This would revert a patch I submitted two years ago (<a href="http://git.xiph.org/?p=opus.git;a=commitdiff;h=1d60b49e9d95672a17ebe5578319c59fa3963224" target="_blank">http://git.xiph.org/?p=opus.git;a=commitdiff;h=1d60b49e9d95672a17ebe5578319c59fa3963224</a>).</div>
<div><br>
</div>
<div>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?</div>
<div><br>
</div>
<div>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.) </div>
<div><br>
</div>
<div>
<blockquote type="cite"></blockquote>
</div>
</div>
<div style="word-wrap:break-word">
<div>
<blockquote type="cite">
<div>On Aug 18, 2017, at 12:34 PM, Felicia Lim <<a href="mailto:flim@google.com" target="_blank">flim@google.com</a>> wrote:</div>
<br class="m_4806609671514967972m_-190920853688853449m_-8800483185505812600m_6725329152546787984Apple-interchange-newline">
</blockquote>
</div>
</div>
<div style="word-wrap:break-word">
<div>
<blockquote type="cite">
<div>
<div dir="ltr">
<div><span style="font-size:13px;color:rgb(33,33,33)">Hi,</span></div>
<div><span style="font-size:13px;color:rgb(33,33,33)"><br>
</span></div>
<div><span style="font-size:13px;color:rgb(33,33,33)">Please find attached a patch to fix alignment exceptions. </span><span style="font-size:13px;color:rgb(33,33,33)">Without this change, we were seeing occasional alignment faults
 when using this with clang.</span></div>
<div><span style="font-size:13px;color:rgb(33,33,33)"><br>
</span></div>
<div><span style="font-size:13px;color:rgb(33,33,33)">Thanks,</span></div>
<div><span style="font-size:13px;color:rgb(33,33,33)">Felicia</span></div>
</div>
</div>
</blockquote>
</div>
</div>
<div style="word-wrap:break-word">
<div>
<blockquote type="cite">
<div><span id="m_4806609671514967972m_-190920853688853449m_-8800483185505812600m_6725329152546787984cid:15df62dbda7a4b2172f1"><e5c277c.diff></span>_______________________________________________<br>
opus mailing list<br>
<a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a><br>
<a href="http://lists.xiph.org/mailman/listinfo/opus" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div></blockquote></div>