[Pixman] Pixman not building on MacOS X 10.11

[Matt Turner]

On Wed, Nov 18, 2015 at 8:35 PM, Siarhei Siamashka
<siarhei.siamashka at gmail.com> wrote:
> On Wed, 18 Nov 2015 14:22:09 -0800
> Matt Turner <mattst88 at gmail.com> wrote:
>
>> On Sun, Oct 11, 2015 at 10:34 AM, Andrea Canciani <ranma42 at gmail.com> wrote:
>> > On Sun, Oct 11, 2015 at 5:30 AM, Siarhei Siamashka
>> > <siarhei.siamashka at gmail.com> wrote:
>> >>
>> >> On Sun, 11 Oct 2015 04:53:08 +0300
>> >> Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:
>> >>
>> >> > On Sat, 10 Oct 2015 16:03:53 -0700
>> >> > Jeremy Huddleston Sequoia <jeremyhu at freedesktop.org> wrote:
>> >> >
>> >> > > > On Oct 10, 2015, at 13:48, Andrea Canciani <ranma42 at 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.
>
> Oh, thanks. Though it is always a good idea to give a short notice
> before landing patches to git, especially controversial ones.
>
> What we had in pixman-0.33.4 was not exactly bad. Sure, GCC did not
> detect MMX support, but the other compilers had no problems with it.
> In a way, this was a good thing because GCC is known to have a broken
> _mm_empty() intrinsic handling and it is probably not going to be
> ever fixed:
>
>     https://gcc.gnu.org/PR47759
>
> In the case of pixman, it happened to cause problems on more than
> one occasion. We do have a test in the test suite, which can detect
> x87 registers corruption caused by a misplaced EMMS instruction due
> to compiler optimizations. But not everyone runs the test suite for
> 32-bit pixman builds on x86 with PIXMAN_DISABLE="sse2 ssse3"
> regularly enough.
>
> It is not like we want to retire the MMX support just because it
> is old and out of fashion. The reason is that it is a source of
> recurring troubles. Sigh. Let's see how long will it take until
> the MMX code breaks again with one compiler version or another.

I had a look at the GCC code for handling -mmmx/-m3dnow flags. It
doesn't look difficult to add a flag for -munion-of-sse-and-3dnowa
(-mmmx2?). I think I'll give that a try, but the most difficult part
might finding a good name. :)