<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>