On Sun, Oct 11, 2015 at 10:34 AM, Andrea Canciani <ranm...@gmail.com> wrote: > On Sun, Oct 11, 2015 at 5:30 AM, Siarhei Siamashka > <siarhei.siamas...@gmail.com> wrote: >> >> On Sun, 11 Oct 2015 04:53:08 +0300 >> Siarhei Siamashka <siarhei.siamas...@gmail.com> wrote: >> >> > On Sat, 10 Oct 2015 16:03:53 -0700 >> > Jeremy Huddleston Sequoia <jerem...@freedesktop.org> wrote: >> > >> > > > On Oct 10, 2015, at 13:48, Andrea Canciani <ranm...@gmail.com> >> > > > wrote: >> > > > The attached hack gets the code to compile on modern clang, but I >> > > > believe first of all we should improve the configure.ac detection >> > > > code >> > > > so that pixman can actually build both on old and on new clang >> > > > versions (possibly with mmx disabled, if the asm constraints we need >> > > > are not implemented). >> > >> > This workaround looks reasonable to me. We should probably just drop >> > the whole "ifdef __OPTIMIZE__" part in >> > >> > http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n92 >> > >> > I don't quite like the fact that this way of returning results from >> > a macro is a GNU C specific extension. But as you said, the configure >> > test can be updated to better match the code and also check if the >> > compiler supports this particular construct. >> > >> > Could you please submit the final variant of your patch in a >> > "git format-patch" format with a commit message and your >> > Signed-off-by tag? >> >> After looking at this issue a bit more, I realized that we are >> about to add a second layer of workarounds on top of the existing >> old workarounds :-) > > > The attached patch should fix the issue with only minor changes. > It keeps the workarounds :( but somewhat it simplifies them :) > I followed your suggestion of checking&using block expressions. > 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. > >> The original problem is that certain compilers (just GCC?) did not >> support some intrinsics when compiling MMX code (_mm_movemask_pi8, >> _mm_mulhi_pu16, _mm_shuffle_pi16) and we got the following code: >> >> http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n66 >> >> In fact, these instructions were not available as part of the original >> MMX, but only got introduced later with AMD Extended 3DNow! and Intel >> SSE1. This is mentioned in the commit messages: >> >> http://cgit.freedesktop.org/pixman/commit/?id=84221f4c1687b8ea14e9cbdc78b2ba7258e62c9e >> >> http://cgit.freedesktop.org/pixman/commit/?id=14208344964f341a7b4a704b05cf4804c23792e9 >> >> These extra instructions are unofficially known as MMX2. But GCC does >> not have a separate option for "-mmmx2". Instead the GCC manual says >> that these intrinsics are available when either "-msse" or a >> combination of "-m3dnow -march=athlon" is used: >> >> https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/x86-Built-in-Functions.html#x86-Built-in-Functions >> >> >> Now I wonder if the comment "We have to compile with -msse to use >> xmmintrin.h" is still valid. I tried to tweak the following ifdef to >> use the part of code, which includes <xmmintrin.h> and the it compiled >> fine for me with CFLAGS="-O2 -m32" using recent versions of GCC and >> Clang: >> >> http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n63 >> >> I believe that this might be somehow related to the new __ALL_ISA__ >> define, which had been mentioned in 2013: >> https://gcc.gnu.org/ml/gcc-patches/2013-04/txts5M0c0uU9y.txt >> >> So what about just dropping this ugly stuff and adding a configure >> check, which would verify if the MMX code can include <xmmintrin.h>? > > > I would love getting rid of the workarounds, but I'm somewhat worried about > the possibility of regressions. > If you believe is a valid option, we might definitely try to pursue it. > > What is the best way forward?
I've now reverted my commit and pushed yours. Thanks. _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman