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

Emil Velikov emil.l.velikov at gmail.com
Fri Apr 29 10:22:25 UTC 2016 

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.
 - Adds separate (x86 and x86-64) SSE4.1 flags and libraries - not
needed afaict, drop ?
 - 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]

Based on the above this should (ideally) be 3 patches. There are some
further comment below - please take a look.

> diff --git a/Android.common.mk b/Android.common.mk
> index d6bf717..93c1465 100644
> --- a/Android.common.mk
> +++ b/Android.common.mk
> @@ -64,13 +64,28 @@ LOCAL_CFLAGS += \
>  LOCAL_CONLYFLAGS += \
>         -std=c99
>
> +x86_flags := \
> +       -DUSE_SSE41 \
> +
> +x86_64_flags := \
> +       -DUSE_SSE41 \
> +
No need for separate x86/x86_64 variables here. If one does not force
the define (as currently done) that is.

>  ifeq ($(strip $(MESA_ENABLE_ASM)),true)
> -ifeq ($(TARGET_ARCH),x86)
> -LOCAL_CFLAGS += \
> +x86_flags += \
>         -DUSE_X86_ASM \
> +       -DUSE_MMX_ASM \
> +       -DUSE_3DNOW_ASM \
> +       -DUSE_SSE_ASM \
> +
> +x86_64_flags += \
> +       -DUSE_X86_64_ASM \
>
These are unterminated (i.e. trailing \).

>  endif
> -endif
> +
> +LOCAL_ASFLAGS_x86 += $(x86_flags)
> +LOCAL_ASFLAGS_x86_64 += $(x86_64_flags)
> +LOCAL_CFLAGS_x86 += $(x86_flags)
> +LOCAL_CFLAGS_x86_64 += $(x86_64_flags)
>
>  ifeq ($(MESA_ENABLE_LLVM),true)
>  LOCAL_CFLAGS += \
> diff --git a/Android.mk b/Android.mk
> index 67d894f..368d02e 100644
> --- a/Android.mk
> +++ b/Android.mk
> @@ -63,12 +63,7 @@ $(warning invalid GPU drivers: $(invalid_drivers))
>  MESA_GPU_DRIVERS := $(filter-out $(invalid_drivers), $(MESA_GPU_DRIVERS))
>  endif
>
> -# host and target must be the same arch to generate matypes.h
> -ifeq ($(TARGET_ARCH),$(HOST_ARCH))
>  MESA_ENABLE_ASM := true
Since now it's always enabled one can drop the variable.

> -else
> -MESA_ENABLE_ASM := false
> -endif
>
>  ifneq ($(filter $(classic_drivers), $(MESA_GPU_DRIVERS)),)
>  MESA_BUILD_CLASSIC := true
> diff --git a/src/mapi/Android.mk b/src/mapi/Android.mk
> index 4445218..b654c25 100644
> --- a/src/mapi/Android.mk
> +++ b/src/mapi/Android.mk
> @@ -57,6 +57,9 @@ intermediates := $(call local-generated-sources-dir)
>  abi_header := $(intermediates)/$(abi_header)
>  LOCAL_GENERATED_SOURCES := $(abi_header)
>
> +# workaround build warning
> +LOCAL_LDFLAGS_x86 += -Wl,--no-warn-shared-textrel
> +
Can you elaborate a bit more here. It might be a genuine bug that we
want to fix.
This is currently hidden amongst many other things which makes it very
easy to miss. If one is to include it with another patch it should be
mentioned in the commit summary.

>  $(abi_header): PRIVATE_PRINTER := shared-glapi
>
>  mapi_abi_headers += $(abi_header)
> diff --git a/src/mesa/Android.gen.mk b/src/mesa/Android.gen.mk
> index e567102..e04482b 100644
> --- a/src/mesa/Android.gen.mk
> +++ b/src/mesa/Android.gen.mk
> @@ -45,10 +45,11 @@ LOCAL_SRC_FILES := $(filter-out $(sources), $(LOCAL_SRC_FILES))
>  LOCAL_C_INCLUDES += $(intermediates)/main
>
>  ifeq ($(strip $(MESA_ENABLE_ASM)),true)
> -ifeq ($(TARGET_ARCH),x86)
> -sources += x86/matypes.h
> -LOCAL_C_INCLUDES += $(intermediates)/x86
> -endif
> +LOCAL_GENERATED_SOURCES_x86 += $(addprefix $(intermediates)/, x86/matypes.h)
> +LOCAL_GENERATED_SOURCES_x86_64 += $(addprefix $(intermediates)/, x86_64/matypes.h)
> +
> +LOCAL_C_INCLUDES_x86 += $(intermediates)/x86
> +LOCAL_C_INCLUDES_x86_64 += $(intermediates)/x86_64
>  endif
>
>  sources += main/git_sha1.h
> @@ -79,12 +80,22 @@ $(intermediates)/main/git_sha1.h: $(wildcard $(MESA_TOP)/.git/HEAD)
>                         > $@; \
>                 fi
>
> -matypes_deps := \
> -       $(BUILD_OUT_EXECUTABLES)/mesa_gen_matypes$(BUILD_EXECUTABLE_SUFFIX) \
> +matypes_deps32 := \
> +       $(BUILD_OUT_EXECUTABLES)/mesa_gen_matypes32$(BUILD_EXECUTABLE_SUFFIX) \
> +       $(LOCAL_PATH)/main/mtypes.h \
> +       $(LOCAL_PATH)/tnl/t_context.h
> +
> +matypes_deps64 := \
> +       $(BUILD_OUT_EXECUTABLES)/mesa_gen_matypes64$(BUILD_EXECUTABLE_SUFFIX) \
>         $(LOCAL_PATH)/main/mtypes.h \
>         $(LOCAL_PATH)/tnl/t_context.h
>
> -$(intermediates)/x86/matypes.h: $(matypes_deps)
> +$(intermediates)/x86/matypes.h: $(matypes_deps32)
> +       @mkdir -p $(dir $@)
> +       @echo "MATYPES: $(PRIVATE_MODULE) <= $(notdir $@)"
> +       $(hide) $< > $@
> +
> +$(intermediates)/x86_64/matypes.h: $(matypes_deps64)
>         @mkdir -p $(dir $@)
>         @echo "MATYPES: $(PRIVATE_MODULE) <= $(notdir $@)"
>         $(hide) $< > $@
Have you've tried building this on a non x86 based system ? Does the
binaries work properly afterwords ?

> diff --git a/src/mesa/Android.libmesa_dricore.mk b/src/mesa/Android.libmesa_dricore.mk
> index d7647a7..6773b5b 100644
> --- a/src/mesa/Android.libmesa_dricore.mk
> +++ b/src/mesa/Android.libmesa_dricore.mk
> @@ -31,6 +31,7 @@ LOCAL_PATH := $(call my-dir)
>  # Import the following variables:
>  #     MESA_FILES
>  #     X86_FILES
> +#     X86_64_FILES
>  include $(LOCAL_PATH)/Makefile.sources
>
>  include $(CLEAR_VARS)
> @@ -42,19 +43,10 @@ LOCAL_SRC_FILES := \
>         $(MESA_FILES)
>
>  ifeq ($(strip $(MESA_ENABLE_ASM)),true)
> -ifeq ($(TARGET_ARCH),x86)
> -       LOCAL_SRC_FILES += $(X86_FILES)
> -endif # x86
> +       LOCAL_SRC_FILES_x86 += $(X86_FILES)
> +       LOCAL_SRC_FILES_x86_64 += $(X86_64_FILES)
>  endif # MESA_ENABLE_ASM
>
> -ifeq ($(ARCH_X86_HAVE_SSE4_1),true)
> -LOCAL_WHOLE_STATIC_LIBRARIES := \
> -       libmesa_sse41
> -LOCAL_CFLAGS := \
> -       -msse4.1 \
> -       -DUSE_SSE41
> -endif
> -
>  LOCAL_C_INCLUDES := \
>         $(MESA_TOP)/src/mapi \
>         $(MESA_TOP)/src/mesa/main \
> @@ -65,6 +57,12 @@ LOCAL_C_INCLUDES := \
>  LOCAL_WHOLE_STATIC_LIBRARIES += \
>         libmesa_program
>
> +LOCAL_WHOLE_STATIC_LIBRARIES_x86 += \
> +       libmesa_sse41 \
> +
> +LOCAL_WHOLE_STATIC_LIBRARIES_x86_64 += \
> +       libmesa_sse41 \
> +
Similar to the defines - one should not need the separate variables
(libraries) and/or hardcoded behaviour.

>  include $(LOCAL_PATH)/Android.gen.mk
>  include $(MESA_COMMON_MK)
>  include $(BUILD_STATIC_LIBRARY)
> diff --git a/src/mesa/Android.libmesa_sse41.mk b/src/mesa/Android.libmesa_sse41.mk
> index 8562da6..3668785 100644
> --- a/src/mesa/Android.libmesa_sse41.mk
> +++ b/src/mesa/Android.libmesa_sse41.mk
> @@ -20,7 +20,7 @@
>  # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>  # DEALINGS IN THE SOFTWARE.
>
> -ifeq ($(ARCH_X86_HAVE_SSE4_1),true)
> +ifneq ($(filter x86 x86_64,$(TARGET_ARCH)),)
>
>  LOCAL_PATH := $(call my-dir)
>
> @@ -33,6 +33,9 @@ LOCAL_MODULE := libmesa_sse41
>  LOCAL_SRC_FILES += \
>         $(X86_SSE41_FILES)
>
> +LOCAL_CFLAGS += \
> +       -msse4.1 \
> +
Trailing \

>  LOCAL_C_INCLUDES := \
>         $(MESA_TOP)/src/mapi \
>         $(MESA_TOP)/src/gallium/include \
> diff --git a/src/mesa/Android.libmesa_st_mesa.mk b/src/mesa/Android.libmesa_st_mesa.mk
> index bbd3956..fc97a0a 100644
> --- a/src/mesa/Android.libmesa_st_mesa.mk
> +++ b/src/mesa/Android.libmesa_st_mesa.mk
> @@ -41,18 +41,10 @@ LOCAL_SRC_FILES := \
>         $(MESA_GALLIUM_FILES)
>
>  ifeq ($(strip $(MESA_ENABLE_ASM)),true)
> -ifeq ($(TARGET_ARCH),x86)
> -       LOCAL_SRC_FILES += $(X86_FILES)
> -endif # x86
> +       LOCAL_SRC_FILES_x86 += $(X86_FILES)
> +       LOCAL_SRC_FILES_x86_64 += $(X86_64_FILES)
>  endif # MESA_ENABLE_ASM
>
> -ifeq ($(ARCH_X86_HAVE_SSE4_1),true)
> -LOCAL_WHOLE_STATIC_LIBRARIES := \
> -       libmesa_sse41
> -LOCAL_CFLAGS := \
> -       -DUSE_SSE41
> -endif
> -
>  LOCAL_C_INCLUDES := \
>         $(MESA_TOP)/src/mapi \
>         $(MESA_TOP)/src/mesa/main \
> @@ -61,7 +53,13 @@ LOCAL_C_INCLUDES := \
>         $(MESA_TOP)/src/gallium/include
>
>  LOCAL_WHOLE_STATIC_LIBRARIES += \
> -       libmesa_program
> +       libmesa_program \
> +
> +LOCAL_WHOLE_STATIC_LIBRARIES_x86 += \
> +       libmesa_sse41 \
> +
> +LOCAL_WHOLE_STATIC_LIBRARIES_x86_64 += \
> +       libmesa_sse41 \
>
>  include $(LOCAL_PATH)/Android.gen.mk
>  include $(MESA_COMMON_MK)
> diff --git a/src/mesa/Android.mesa_gen_matypes.mk b/src/mesa/Android.mesa_gen_matypes.mk
> index 4fcf73a..163f0e2 100644
> --- a/src/mesa/Android.mesa_gen_matypes.mk
> +++ b/src/mesa/Android.mesa_gen_matypes.mk
> @@ -25,13 +25,16 @@
>  # ---------------------------------------------------------------------
>
>  ifeq ($(strip $(MESA_ENABLE_ASM)),true)
> -ifeq ($(TARGET_ARCH),x86)
> +ifneq ($(filter x86 x86_64,$(TARGET_ARCH)),)
>
>  LOCAL_PATH := $(call my-dir)
>
>  include $(CLEAR_VARS)
>
>  LOCAL_MODULE := mesa_gen_matypes
> +LOCAL_MULTILIB := both
> +LOCAL_MODULE_STEM_32 := $(LOCAL_MODULE)32
> +LOCAL_MODULE_STEM_64 := $(LOCAL_MODULE)64
>  LOCAL_IS_HOST_MODULE := true
>
>  LOCAL_C_INCLUDES := \
> @@ -43,5 +46,5 @@ LOCAL_SRC_FILES := \
>  include $(MESA_COMMON_MK)
>  include $(BUILD_HOST_EXECUTABLE)
>
> -endif # x86
> +endif # x86 x86_64
>  endif # MESA_ENABLE_ASM
> diff --git a/src/mesa/drivers/dri/i965/Android.mk b/src/mesa/drivers/dri/i965/Android.mk
> index 056b223..a82b69b 100644
> --- a/src/mesa/drivers/dri/i965/Android.mk
> +++ b/src/mesa/drivers/dri/i965/Android.mk
> @@ -39,11 +39,6 @@ include $(LOCAL_PATH)/Makefile.sources
>  LOCAL_CFLAGS := \
>         $(MESA_DRI_CFLAGS)
>
> -ifeq ($(ARCH_X86_HAVE_SSE4_1),true)
> -LOCAL_CFLAGS += \
> -       -DUSE_SSE41
> -endif
> -
>  LOCAL_C_INCLUDES := \
>         $(MESA_DRI_C_INCLUDES)
>
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "Android-x86 development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to android-x86-devel+unsubscribe at googlegroups.com.
> To post to this group, send email to android-x86-devel at googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/android-x86-devel/1461828900-4813-7-git-send-email-cwhuang%40linux.org.tw.
> For more options, visit https://groups.google.com/d/optout.

More information about the mesa-dev mailing list