[RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[George Spelvin]

Assembly of 32-bit multiply/divide helpers was dependent on
CONFIG_CPU_HAS_NO_MULDIV64, which is not necessary.

Signed-off-by: George Spelvin <linux@horizon.com>
---
This also turns out to be a very useful symbol for some other work
I'm doing to <linux/hash.h>.

 arch/m68k/Kconfig.cpu  | 11 +++++++++++
 arch/m68k/lib/Makefile |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
index 0dfcf12..3fce7bf 100644
--- a/arch/m68k/Kconfig.cpu
+++ b/arch/m68k/Kconfig.cpu
@@ -37,6 +37,7 @@ config M68000
 	bool "MC68000"
 	depends on !MMU
 	select CPU_HAS_NO_BITFIELDS
+	select CPU_HAS_NO_MULDIV32
 	select CPU_HAS_NO_MULDIV64
 	select CPU_HAS_NO_UNALIGNED
 	select GENERIC_CSUM
@@ -391,8 +392,18 @@ config NODES_SHIFT
 config CPU_HAS_NO_BITFIELDS
 	bool
 
+config CPU_HAS_NO_MULDIV32
+	bool
+	help
+	  This option indicates the CPU lacks 32x32->32-bit multiply
+	  and divide instructions, and thus requires the GCC helper
+	  functions __mulsi3, __divsi3 __modsi3, etc.
+
 config CPU_HAS_NO_MULDIV64
 	bool
+	help
+	  This option indicates the CPU lacks "widening" 32x32->64-bit
+	  multiply and 64/32->(32,32)-bit divide instructions.
 
 config CPU_HAS_NO_UNALIGNED
 	bool
diff --git a/arch/m68k/lib/Makefile b/arch/m68k/lib/Makefile
index fcd8eb1..e6553db 100644
--- a/arch/m68k/lib/Makefile
+++ b/arch/m68k/lib/Makefile
@@ -7,8 +7,8 @@ lib-y	:= ashldi3.o ashrdi3.o lshrdi3.o muldi3.o \
 	   memcpy.o memset.o memmove.o
 
 lib-$(CONFIG_MMU) += uaccess.o
-lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += mulsi3.o divsi3.o udivsi3.o
-lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += modsi3.o umodsi3.o
+lib-$(CONFIG_CPU_HAS_NO_MULDIV32) += mulsi3.o divsi3.o udivsi3.o
+lib-$(CONFIG_CPU_HAS_NO_MULDIV32) += modsi3.o umodsi3.o
 
 ifndef CONFIG_GENERIC_CSUM
 lib-y	+= checksum.o
-- 
2.8.1

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[Greg Ungerer]

Hi George,

On 11/05/16 20:25, George Spelvin wrote:
> Assembly of 32-bit multiply/divide helpers was dependent on
> CONFIG_CPU_HAS_NO_MULDIV64, which is not necessary.
>
> Signed-off-by: George Spelvin <linux@horizon.com>
> ---
> This also turns out to be a very useful symbol for some other work
> I'm doing to <linux/hash.h>.
>
>   arch/m68k/Kconfig.cpu  | 11 +++++++++++
>   arch/m68k/lib/Makefile |  4 ++--
>   2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
> index 0dfcf12..3fce7bf 100644
> --- a/arch/m68k/Kconfig.cpu
> +++ b/arch/m68k/Kconfig.cpu
> @@ -37,6 +37,7 @@ config M68000
>   	bool "MC68000"
>   	depends on !MMU
>   	select CPU_HAS_NO_BITFIELDS
> +	select CPU_HAS_NO_MULDIV32
>   	select CPU_HAS_NO_MULDIV64
>   	select CPU_HAS_NO_UNALIGNED
>   	select GENERIC_CSUM
> @@ -391,8 +392,18 @@ config NODES_SHIFT
>   config CPU_HAS_NO_BITFIELDS
>   	bool
>
> +config CPU_HAS_NO_MULDIV32
> +	bool
> +	help
> +	  This option indicates the CPU lacks 32x32->32-bit multiply
> +	  and divide instructions, and thus requires the GCC helper
> +	  functions __mulsi3, __divsi3 __modsi3, etc.
> +
>   config CPU_HAS_NO_MULDIV64
>   	bool
> +	help
> +	  This option indicates the CPU lacks "widening" 32x32->64-bit
> +	  multiply and 64/32->(32,32)-bit divide instructions.
>
>   config CPU_HAS_NO_UNALIGNED
>   	bool
> diff --git a/arch/m68k/lib/Makefile b/arch/m68k/lib/Makefile
> index fcd8eb1..e6553db 100644
> --- a/arch/m68k/lib/Makefile
> +++ b/arch/m68k/lib/Makefile
> @@ -7,8 +7,8 @@ lib-y	:= ashldi3.o ashrdi3.o lshrdi3.o muldi3.o \
>   	   memcpy.o memset.o memmove.o
>
>   lib-$(CONFIG_MMU) += uaccess.o
> -lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += mulsi3.o divsi3.o udivsi3.o
> -lib-$(CONFIG_CPU_HAS_NO_MULDIV64) += modsi3.o umodsi3.o
> +lib-$(CONFIG_CPU_HAS_NO_MULDIV32) += mulsi3.o divsi3.o udivsi3.o
> +lib-$(CONFIG_CPU_HAS_NO_MULDIV32) += modsi3.o umodsi3.o

Did you intend to no longer compile any of mulsi3.o, etc, for
ColdFire targets?

CPU_HAS_NO_MULDIV64 is selected by both M68000 and ColdFire, so
those functions used to be compiled. But with this change on
only M68000 will compile them.

That causes a problem if we are forced to fallback and use the
gcc -m5200 cpu switch. (That may be the case if the version of
gcc we are compiling with doesn't support the more advanced
ColdFire CPU selection switches).

When compiling with m5200 gcc will generate calls to divsi3,
udivsi3, modsi3 and umodsi3. (As far as I can tell we never need
mulsi3 for ColdFire). So linking will fail with this patch as-is
in that m5200 case.

Regards
Greg

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[George Spelvin]

> Did you intend to no longer compile any of mulsi3.o, etc, for
> ColdFire targets?
>
> CPU_HAS_NO_MULDIV64 is selected by both M68000 and ColdFire, so
> those functions used to be compiled. But with this change on
> only M68000 will compile them.

Yes, that's exactly the point.  ColdFire doesn't need or use them.

This was baiscally my answer to "why is there a ColdFire path
in __mulsi3"?

> When compiling with m5200 gcc will generate calls to divsi3,
> udivsi3, modsi3 and umodsi3. (As far as I can tell we never need
> mulsi3 for ColdFire). So linking will fail with this patch as-is
> in that m5200 case.

Ah!  I missed that on my testing.  Yes, indeed, there is *one* ColdFire
that needs the divides.  And -mcpu=5206 is a supported option.

I had tested with

m68k-linux-gnu-gcc-6 -march=isaa -fomit-frame-pointer -S mul.c

unsigned mul(unsigned x, unsigned y) { return x * y; }
unsigned div(unsigned x, unsigned y) { return x / y; }
unsigned rem(unsigned x, unsigned y) { return x % y; }
int sdiv(int x, int y) { return x / y; }
int srem(int x, int y) { return x % y; }

But yes, -m5206 isn't the same as "-march=isaa -mtune=5200"; the very
first is "ISA A minus" and doesn't have the *divides*.

Now that you point it out, I also see the -m5200 fallback in the Makefile.


Grumble, that just got more complicated.  It's easy enough to split
the option into CPU_HAS_NO_MUL32 and CPU_HAS_NO_DIV32, but the answer
depends on the GCC version, not just the config settings.

Options include:
- Punt on the divide part entirely, and just always compile divides like now.
  It's just a tiny bit of code space.
- (falsely) set CPU_HAS_NO_DIV32 for those CPUs always.
  The problem is, some future code might make optimization decisions
  based on that (e.g. using binary vs. Euclidean GCD algorithms),
  and for the 99% of people using a modern compiler, it'll be wrong.
- Do some post-Kconfig Makefile magic to detect the situation.

How ugly is this:

+lib-$(CONFIG_CPU_HAS_NO_MUL32) += mulsi3.o
+lib-$(CONFIG_CPU_HAS_NO_DIV32) += divsi3.o udivsi3.o modsi3.o umodsi3.o
+
+# Old GCC versions fall back to -m5200 compilation, generating these calls
+# even though the CPU doesn't actually need it.  See arch/m68k/Makefile.
+
+ifeq ($(cpuflags-y),-m5200)
+lib-y += divsi3.o udivsi3.o modsi3.o umodsi3.o
+endif

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[Greg Ungerer]

Hi George,

On 13/05/16 06:31, George Spelvin wrote:
>> Did you intend to no longer compile any of mulsi3.o, etc, for
>> ColdFire targets?
>>
>> CPU_HAS_NO_MULDIV64 is selected by both M68000 and ColdFire, so
>> those functions used to be compiled. But with this change on
>> only M68000 will compile them.
> 
> Yes, that's exactly the point.  ColdFire doesn't need or use them.
> 
> This was baiscally my answer to "why is there a ColdFire path
> in __mulsi3"?
> 
>> When compiling with m5200 gcc will generate calls to divsi3,
>> udivsi3, modsi3 and umodsi3. (As far as I can tell we never need
>> mulsi3 for ColdFire). So linking will fail with this patch as-is
>> in that m5200 case.
> 
> Ah!  I missed that on my testing.  Yes, indeed, there is *one* ColdFire
> that needs the divides.  And -mcpu=5206 is a supported option.
> 
> I had tested with
> 
> m68k-linux-gnu-gcc-6 -march=isaa -fomit-frame-pointer -S mul.c
> 
> unsigned mul(unsigned x, unsigned y) { return x * y; }
> unsigned div(unsigned x, unsigned y) { return x / y; }
> unsigned rem(unsigned x, unsigned y) { return x % y; }
> int sdiv(int x, int y) { return x / y; }
> int srem(int x, int y) { return x % y; }
> 
> But yes, -m5206 isn't the same as "-march=isaa -mtune=5200"; the very
> first is "ISA A minus" and doesn't have the *divides*.

It is rather annoying that the baseline isaa isn't really
the baseline :-)


> Now that you point it out, I also see the -m5200 fallback in the Makefile.
> 
> 
> Grumble, that just got more complicated.  It's easy enough to split
> the option into CPU_HAS_NO_MUL32 and CPU_HAS_NO_DIV32, but the answer
> depends on the GCC version, not just the config settings.
> 
> Options include:
> - Punt on the divide part entirely, and just always compile divides like now.
>   It's just a tiny bit of code space.

That part is no big deal. Those functions are library linked,
so if the final kernel doesn't need them then they will not
end up taking any code space at all.


> - (falsely) set CPU_HAS_NO_DIV32 for those CPUs always.
>   The problem is, some future code might make optimization decisions
>   based on that (e.g. using binary vs. Euclidean GCD algorithms),
>   and for the 99% of people using a modern compiler, it'll be wrong.

Yeah, that would not be so good. I would always want to assume
the presence of hardware div/rem for ColdFire (since all the hardware
we currently run on actually does have it). And the further in
the future you go the less likely your are to be using a really
old compiler.


> - Do some post-Kconfig Makefile magic to detect the situation.
> 
> How ugly is this:
> 
> +lib-$(CONFIG_CPU_HAS_NO_MUL32) += mulsi3.o
> +lib-$(CONFIG_CPU_HAS_NO_DIV32) += divsi3.o udivsi3.o modsi3.o umodsi3.o
> +
> +# Old GCC versions fall back to -m5200 compilation, generating these calls
> +# even though the CPU doesn't actually need it.  See arch/m68k/Makefile.
> +
> +ifeq ($(cpuflags-y),-m5200)
> +lib-y += divsi3.o udivsi3.o modsi3.o umodsi3.o
> +endif

I think I could live with that.

Regards
Greg

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[Finn Thain]

On Fri, 12 May 2016, George Spelvin wrote:

> 
> +lib-$(CONFIG_CPU_HAS_NO_MUL32) += mulsi3.o
> +lib-$(CONFIG_CPU_HAS_NO_DIV32) += divsi3.o udivsi3.o modsi3.o umodsi3.o
> +
> +# Old GCC versions fall back to -m5200 compilation, generating these calls
> +# even though the CPU doesn't actually need it.  See arch/m68k/Makefile.
> +
> +ifeq ($(cpuflags-y),-m5200)
> +lib-y += divsi3.o udivsi3.o modsi3.o umodsi3.o
> +endif

The comment is vague. Does anyone know which GCC versions do or don't work 
like this?

-- 

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[George Spelvin]

Finn Thain wrote:
> On Fri, 12 May 2016, George Spelvin wrote:
>> +# Old GCC versions fall back to -m5200 compilation, generating these calls
>> +# even though the CPU doesn't actually need it.  See arch/m68k/Makefile.
>
> The comment is vague. Does anyone know which GCC versions do or don't work 
> like this?

Yes, I looked it up in the archived GCC documentation.  My current patch says:

(commit comment)
    Since the compiler version is not known at Kconfig time, this cannot
    be expressed in a CONFIG_ variable, but instead is handled by some
    Makefile hackery.  This only applies to GCC 4.2.4 and earlier, which
    is hopefully almost everyone, but Documentation/Changes says GCC 3.2.

(arch/m68k/lib/Makefile)
+# GCC 4.2.4 and earlier don't know about ColdFire models that support
+# DIV.L and fall back to -m5200, generating these calls even though the
+# CPU doesn't actually need it.  Not needed on GCC 4.3 or later.
+# See the $(call cc-option ...) lines in arch/m68k/Makefile.

Re: [RFC PATCH 2/2] arch/m68k: Add CONFIG_CPU_HAS_NO_MULDIV32

[Finn Thain]

On Fri, 12 May 2016, George Spelvin wrote:

> Finn Thain wrote:
> > On Fri, 12 May 2016, George Spelvin wrote:
> >> +# Old GCC versions fall back to -m5200 compilation, generating these calls
> >> +# even though the CPU doesn't actually need it.  See arch/m68k/Makefile.
> >
> > The comment is vague. Does anyone know which GCC versions do or don't 
> > work like this?
> 
> Yes, I looked it up in the archived GCC documentation.

Thanks.

>  My current patch says:
> 
> (commit comment)
>     Since the compiler version is not known at Kconfig time, this cannot 
>     be expressed in a CONFIG_ variable, but instead is handled by some 
>     Makefile hackery.  This only applies to GCC 4.2.4 and earlier, which 
>     is hopefully almost everyone, but Documentation/Changes says GCC 
>     3.2.

I think you meant to write, "This does not apply to GCC 4.3 or later", 
as that's what you put in the new comment below. 

> 
> (arch/m68k/lib/Makefile)
> +# GCC 4.2.4 and earlier don't know about ColdFire models that support
> +# DIV.L and fall back to -m5200, generating these calls even though the
> +# CPU doesn't actually need it.  Not needed on GCC 4.3 or later.
> +# See the $(call cc-option ...) lines in arch/m68k/Makefile.

--