<div dir="ltr">On Sun, Oct 11, 2015 at 5:30 AM, Siarhei Siamashka <span dir="ltr"><<a href="mailto:siarhei.siamashka@gmail.com" target="_blank">siarhei.siamashka@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On Sun, 11 Oct 2015 04:53:08 +0300<br>
Siarhei Siamashka <<a href="mailto:siarhei.siamashka@gmail.com">siarhei.siamashka@gmail.com</a>> wrote:<br>
<br>
> On Sat, 10 Oct 2015 16:03:53 -0700<br>
> Jeremy Huddleston Sequoia <<a href="mailto:jeremyhu@freedesktop.org">jeremyhu@freedesktop.org</a>> wrote:<br>
><br>
> > > On Oct 10, 2015, at 13:48, Andrea Canciani <<a href="mailto:ranma42@gmail.com">ranma42@gmail.com</a>> wrote:<br>
</span><span class="">> > > The attached hack gets the code to compile on modern clang, but I<br>
> > > believe first of all we should improve the <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a> detection code<br>
> > > so that pixman can actually build both on old and on new clang<br>
> > > versions (possibly with mmx disabled, if the asm constraints we need<br>
> > > are not implemented).<br>
><br>
> This workaround looks reasonable to me. We should probably just drop<br>
> the whole "ifdef __OPTIMIZE__" part in<br>
>     <a href="http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n92" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n92</a><br>
><br>
> I don't quite like the fact that this way of returning results from<br>
> a macro is a GNU C specific extension. But as you said, the configure<br>
> test can be updated to better match the code and also check if the<br>
> compiler supports this particular construct.<br>
><br>
> Could you please submit the final variant of your patch in a<br>
> "git format-patch" format with a commit message and your<br>
> Signed-off-by tag?<br>
<br>
</span>After looking at this issue a bit more, I realized that we are<br>
about to add a second layer of workarounds on top of the existing<br>
old workarounds :-)<br></blockquote><div><br></div><div>The attached patch should fix the issue with only minor changes.<br>It keeps the workarounds :( but somewhat it simplifies them :)<br></div><div>I followed your suggestion of checking&using block expressions.<br></div><div>Given that the _mm_shuffle_pi16() function is always used in a "return" statement, if needed we could avoid the usage of block expressions by defining a macro "_return_mm_shuffle_pi16()" (which would return the result of the operation instead of making it available as an expression) both for the xmmintrin branch and for the hand-coded one.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The original problem is that certain compilers (just GCC?) did not<br>
support some intrinsics when compiling MMX code (_mm_movemask_pi8,<br>
_mm_mulhi_pu16, _mm_shuffle_pi16) and we got the following code:<br>
    <a href="http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n66" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n66</a><br>
<br>
In fact, these instructions were not available as part of the original<br>
MMX, but only got introduced later with AMD Extended 3DNow! and Intel<br>
SSE1. This is mentioned in the commit messages:<br>
    <a href="http://cgit.freedesktop.org/pixman/commit/?id=84221f4c1687b8ea14e9cbdc78b2ba7258e62c9e" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/commit/?id=84221f4c1687b8ea14e9cbdc78b2ba7258e62c9e</a><br>
    <a href="http://cgit.freedesktop.org/pixman/commit/?id=14208344964f341a7b4a704b05cf4804c23792e9" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/commit/?id=14208344964f341a7b4a704b05cf4804c23792e9</a><br>
<br>
These extra instructions are unofficially known as MMX2. But GCC does<br>
not have a separate option for "-mmmx2". Instead the GCC manual says<br>
that these intrinsics are available when either "-msse" or a<br>
combination of "-m3dnow -march=athlon" is used:<br>
    <a href="https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/x86-Built-in-Functions.html#x86-Built-in-Functions" rel="noreferrer" target="_blank">https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/x86-Built-in-Functions.html#x86-Built-in-Functions</a><br>
<br>
<br>
Now I wonder if the comment "We have to compile with -msse to use<br>
xmmintrin.h" is still valid. I tried to tweak the following ifdef to<br>
use the part of code, which includes <xmmintrin.h> and the it compiled<br>
fine for me with CFLAGS="-O2 -m32" using recent versions of GCC and<br>
Clang:<br>
    <a href="http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n63" rel="noreferrer" target="_blank">http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n63</a><br>
<br>
I believe that this might be somehow related to the new __ALL_ISA__<br>
define, which had been mentioned in 2013:<br>
    <a href="https://gcc.gnu.org/ml/gcc-patches/2013-04/txts5M0c0uU9y.txt" rel="noreferrer" target="_blank">https://gcc.gnu.org/ml/gcc-patches/2013-04/txts5M0c0uU9y.txt</a><br>
<br>
So what about just dropping this ugly stuff and adding a configure<br>
check, which would verify if the MMX code can include <xmmintrin.h>?<br></blockquote><div><br></div><div>I would love getting rid of the workarounds, but I'm somewhat worried about the possibility of regressions.<br></div><div>If you believe is a valid option, we might definitely try to pursue it.<br><br></div><div>What is the best way forward?<br><br></div><div>Andrea<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
And if this configure check fails (because of using an old compiler),<br>
then simply disable MMX support when building pixman.<br>
<div class=""><div class="h5"><br>
--<br>
Best regards,<br>
Siarhei Siamashka<br>
</div></div></blockquote></div><br></div></div>