[Mesa-dev] [PATCH] mesa: Fix build to properly check for supported compiler flags

[Matt Turner]

On Thu, Dec 19, 2013 at 1:50 AM, Lauri Kasanen <cand at gmx.com> wrote:
> On Wed, 18 Dec 2013 17:57:16 -0800
> Matt Turner <mattst88 at gmail.com> wrote:
>
>> On Tue, Dec 17, 2013 at 7:15 AM, Lauri Kasanen <cand at gmx.com> wrote:
>> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=72708
>>
>> Make this "Bugzilla: ..."
>
> Sure, will do. But there are both conventions used, for example see:
> 0b16042377a
> 8bc7673ef87
>
>> > It seems to me that the Intel code that uses this SSE4.1 function
>> > is still buggy, as it has no runtime check - would it not crash
>> > if built on a SSE4-capable system but used with a lower-class cpu?
>>
>> You haven't thought this through. :)
>>
>> The SSE 4.1 code is in i965 #ifdef __SSE4_1__. __SSE4_1__ is defined
>> if the code is compiled with the appropriate CFLAGS, e.g., -msse4.1 or
>> -march=native and your CPU support SSE 4.1.
>>
>> That is, your Mesa would have to be compiled with flags incompatible
>> with your CPU for this to break, and in that case gcc would already
>> have used instructions your CPU doesn't have.
>
> Yes, you're right that there is no crash risk on a normal build.
> However, it is practically useless for distro builds like that, as they
> can't ship a normal build and get the speedup at the same time.

Right, this is a separate issue though and not one that's solved by this patch.

> Gallium has a cpu detect function that includes a SSE 4.1 cap. Perhaps
> you'd like to move it to a common area and make use of it for runtime
> detection?

Yes, that would be worthwhile.

>> > diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am
>> > index a60600e..8843836 100644
>> > --- a/src/mesa/Makefile.am
>> > +++ b/src/mesa/Makefile.am
>> > @@ -103,7 +103,11 @@ noinst_PROGRAMS = gen_matypes
>> >  gen_matypes_SOURCES = x86/gen_matypes.c
>> >  BUILT_SOURCES += matypes.h
>> >
>> > -ARCH_LIBS = libmesa_sse41.la
>> > +ARCH_LIBS =
>> > +
>> > +if SSE41_SUPPORTED
>> > +ARCH_LIBS += libmesa_sse41.la
>> > +endif
>>
>> It's safe to just assign ARCH_LIBS once and not do the empty assignment.
>
> Yes, but I did it this way to pave way for future additions. Having it
> like this makes it easy to add specialized functions for other
> instruction sets later.

Okay.