Re: PATCH: PRs bootstrap/59580/59583: Improve x86 --with-arch/--with-cpu= configure handling

[Uros Bizjak <ubizjak at gmail dot com>]

On Mon, Dec 23, 2013 at 3:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>> Please get someone to review config.gcc changes. They are OK as far as
>> x86 rename is concerned, but I can't review functional changes.
>
> Hi Paolo,
>
> Can you review this config.gcc change?
>
>>
>> > @@ -588,6 +588,22 @@ esac
>> >  # Common C libraries.
>> >  tm_defines="$tm_defines LIBC_GLIBC=1 LIBC_UCLIBC=2 LIBC_BIONIC=3"
>> >
>> > +# 32-bit x86 processors supported by --with-arch=.  Each processor
>> > +# MUST be separated by exactly one space.
>> > +x86_archs="athlon athlon-4 athlon-fx athlon-mp athlon-tbird \
>> > +athlon-xp k6 k6-2 k6-3 geode c3 c3-2 winchip-c6 winchip2 i386 i486 \
>> > +i586 i686 pentium pentium-m pentium-mmx pentium2 pentium3 pentium3m \
>> > +pentium4 pentium4m pentiumpro prescott"
>>
>> Missing "native".
>
> x86_archs contains 32-bit x86 processors.  "native" is allowed for
> 64-bit targets and is included in x86_64_archs.  64-bit processors
> can be used in --with-arch/--with-cpu= for 32-bit targets.
>
> Here is a patch to improve x86 x86 --with-arch/--with-cpu= configure
> handling.  This patch defines 3 variables:
>
> 1. x86_archs: It contains 32-bit x86 processors supported by
> --with-arch=, which aren't allowed for 64-bit targets.
> 2. x86_64_archs: It contains 64-bit x86 processors supported by
> --with-arch=, which are allowed for both 32-bit and 64-bit targets.
> 3. x86_cpus.  It contains x86 processors supported by --with-cpu=,
> which are allowed for both 32-bit and 64-bit targets.
>
> Each processor in those 3 variables are separated by exactly one space.
>
> Instead of checking if a value of --with-arch/--with-cpu= is valid in many
> difference places with
>
> case ${val} in
> valid pattern list)
>   OK
>   ;;
> *)
>   error
>   exit 1
>   ;;
> esac
>
> and updating all pattern lists when adding a new processor, this patch
> uses
>
> case " valid processor list separated by exactly one space " in
> *" ${val} "*)
>   OK
>   ;;
> *)
>   error
>   exit 1
>   ;;
> esac
>
> "valid processor list separated by exactly one space" is combination
> of 3 processor variables.  It only needs separate a check for empty
> value with
>
> if test x${val} != x; then
>   $val isn't empty
> else
>   $val is empty
> fi
>
> With this approach, we only need to add new 32-bit processors to x86_archs
> and new 64-bit processors to x86_64_archs.  They will be supported by
> --with-arch/--with-cpu= automatically.  OK to install?
>
> Thanks.
>
>
> H.J.
> ---
> 2013-12-23   H.J. Lu  <hongjiu.lu@intel.com>
>
>         PR bootstrap/59580
>         PR bootstrap/59583
>         * config.gcc (x86_archs): New variable.
>         (x86_64_archs): Likewise.
>         (x86_cpus): Likewise.
>         Use $x86_archs, $x86_64_archs and $x86_cpus to check valid
>         --with-arch/--with-cpu= options.
>         Support --with-arch=/--with-cpu={nehalem,westmere,
>         sandybridge,ivybridge,haswell,broadwell,bonnell,silvermont}.

Probably we can use some regex for whitespace to relax "MUST be
separated by exacly one space" limitation, but nevertheless the patch
looks like a much needed cleanup to me. OTOH, the comment clearly says
what to do when changing strings.

Under the assumption that build maintainers silently agree, and due to
the fact that the patch touches x86 parts only, the patch is OK for
mainline with a couple of nits below.

Thanks,
Uros.

> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 24dbaf9..51eb2b1 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -588,6 +588,22 @@ esac
>  # Common C libraries.
>  tm_defines="$tm_defines LIBC_GLIBC=1 LIBC_UCLIBC=2 LIBC_BIONIC=3"
>
> +# 32-bit x86 processors supported by --with-arch=.  Each processor
> +# MUST be separated by exactly one space.
> +x86_archs="athlon athlon-4 athlon-fx athlon-mp athlon-tbird \
> +athlon-xp k6 k6-2 k6-3 geode c3 c3-2 winchip-c6 winchip2 i386 i486 \
> +i586 i686 pentium pentium-m pentium-mmx pentium2 pentium3 pentium3m \
> +pentium4 pentium4m pentiumpro prescott"

Please put some vertical space here...

> +# 64-bit x86 processors supported by --with-arch=.  Each processor
> +# MUST be separated by exactly one space.
> +x86_64_archs="amdfam10 athlon64 athlon64-sse3 barcelona bdver1 bdver2 \
> +bdver3 bdver4 btver1 btver2 k8 k8-sse3 opteron opteron-sse3 nocona \
> +core2 corei7 corei7-avx core-avx-i core-avx2 atom slm nehalem westmere \
> +sandybridge ivybridge haswell broadwell bonnell silvermont x86-64 native"

... and here.

> +# Additional x86 processors supported by --with-cpu=.  Each processor
> +# MUST be separated by exactly one space.
> +x86_cpus="generic intel"