[Mesa-dev] [android-x86-devel] [RFC 6/7] android: enable x86 asm and sse4 for x86 and x86_64

[Emil Velikov]

On 30 April 2016 at 18:36, Zhen Wu <wuzhen at jidemail.com> wrote:
> 2016-04-29 18:22 GMT+08:00 Emil Velikov <emil.l.velikov at gmail.com>:
>>
>> On 28 April 2016 at 08:34, Chih-Wei Huang <cwhuang at android-x86.org> wrote:
>> > From: WuZhen <wuzhen at jidemail.com>
>> >
>> > Support multilib compilation use runtime feature selection.
>> >
>> > NO_REF_TASK
>> > tested: local run
>> >
>> > Change-Id: Iee7961effdecde09cfbdaf09455bfb0912699ae3
>> > Signed-off-by: Chih-Wei Huang <cwhuang at linux.org.tw>
>> > ---
>> >  Android.common.mk                    | 21 ++++++++++++++++++---
>> >  Android.mk                           |  5 -----
>> >  src/mapi/Android.mk                  |  3 +++
>> >  src/mesa/Android.gen.mk              | 25 ++++++++++++++++++-------
>> >  src/mesa/Android.libmesa_dricore.mk  | 20 +++++++++-----------
>> >  src/mesa/Android.libmesa_sse41.mk    |  5 ++++-
>> >  src/mesa/Android.libmesa_st_mesa.mk  | 20 +++++++++-----------
>> >  src/mesa/Android.mesa_gen_matypes.mk |  7 +++++--
>> >  src/mesa/drivers/dri/i965/Android.mk |  5 -----
>> >  9 files changed, 66 insertions(+), 45 deletions(-)
>> >
>>
>> Afact this patch does a few things:
>>  - Moves -DUSE_SSE41 to the top level - good idea [patch 1]
>>  - Consolidates -msse41 to libmesa_sse41.mk (effectively adding it to
>> st_mesa) - good idea/bugfix.
>> Take a look into configure.ac's add -mstackrealign (and the commit
>> that introduced it). You might want the same/similar fix. [patch 2]
>>
>>  - Forces SSE4.1 even if there is no support for it. I.e.
>> ARCH_X86_HAVE_SSE4_1 checks were dropped - very bad idea, please
>> don't.
>
>
> mesa seems always check for SSE4.1 support before using any of sse4.1
> functions, if you are talking about compiler support for sse4.1, the
> toolchain used in android should always support this. I only changed that
> into always generate sse4.1 and rely on those runtion checks to avoid issues
> on system without sse4.1 support, do you think this is a issue?
>
I'm thinking about a few things here:
 - If someone is to have a hacked kernel which always advertises
SSE4.1, even if it's not done natively but via emulation.
Thus the actual usefulness of these codepaths will be... limited.
 - The patch removes what seems to me a good approach only to replace
it with a hacky version - the x86/x86-64 specific flags.

>>  - Adds separate (x86 and x86-64) SSE4.1 flags and libraries - not
>> needed afaict, drop ?
>
>
> I'm trying to avoid not breaking non-x86 builds.  this  is to seems better
> than to surround them in a if clause. especially if we're to add flags for
> other architectures.
>
The non-x86 build(s) end up broken because the ARCH_X86_HAVE_SSE4_1
check was dropped. Please keep that one and then you won't need this
workaround.

>>  - Has some trailing '\' in which should cause build warning/errors -
>> see comments below
>>  - Gets "asm" building when host_arch != target_arch - have you guys
>> tested it. what is the perf. improvement ? [patch 3]
>
>
> No, we have not. We're trying to be in line to other platforms.
>
Although very admirable that you want to keep things in sync with the
autoconf build, please don't add code that hasn't been tested.
Note: this isn't that strict of a rule about the Android build
although the asm code is very picky/fragile. I'm trying to avoid very
nasty/subtle explosions.

Thanks
Emil