[Bug target/57583] large switches with jump tables are horribly broken on m68k

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

Jeffrey A. Law  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 CC||law at redhat dot com
 Resolution|--- |FIXED

--- Comment #16 from Jeffrey A. Law  ---
Fixed on the trunk.  No current plans to backport to release branches.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #15 from Jeffrey A. Law  ---
Author: law
Date: Fri Jan  6 21:21:02 2017
New Revision: 244184

URL: https://gcc.gnu.org/viewcvs?rev=244184=gcc=rev
Log:
2017-01-06  Mikael Pettersson  

PR target/57583
* config/m68k/m68k.opt (LONG_JUMP_TABLE_OFFSETS): New option.
* config/m68k/linux.h (ASM_RETURN_CASE_JUMP): Handle
TARGET_LONG_JUMP_TABLE_OFFSETS.
* config/m68k/m68kelf.h (ASM_RETURN_CASE_JUMP): Likewise.
* config/m68k/netbsd-elf.h (ASM_RETURN_CASE_JUMP): Likewise.
* config/m68k/m68k.h (CASE_VECTOR_MODE): Likewise.
(ASM_OUTPUT_ADDR_DIFF_ELF): Likewise.
* config/m68k/m68k.md (tablejump expander): Likewise.
(*tablejump_pcrel_hi): Renamed from unnamed insn, reject
TARGET_LONG_JUMP_TABLE_OFFSETS.
(*tablejump_pcrel_si): New insn, handle TARGET_LONG_JUMP_TABLE_OFFSETS.
* doc/invoke.texi (M68K options): Add -mlong-jump-table-offsets.

Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/m68k/linux.h
trunk/gcc/config/m68k/m68k.h
trunk/gcc/config/m68k/m68k.md
trunk/gcc/config/m68k/m68k.opt
trunk/gcc/config/m68k/m68kelf.h
trunk/gcc/config/m68k/netbsd-elf.h
trunk/gcc/doc/invoke.texi

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #14 from Mikael Pettersson  ---
Patch submitted:
https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00419.html

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #13 from John Paul Adrian Glaubitz  ---
(In reply to Mikael Pettersson from comment #12)
> Thanks for testing, I'll submit it for gcc-7 shortly.

Ah, awesome, thanks! Would also be cool to have this backported to the gcc-6
branch if possible.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #12 from Mikael Pettersson  ---
Thanks for testing, I'll submit it for gcc-7 shortly.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #11 from John Paul Adrian Glaubitz  ---
Ok, the patch does indeed work. Tested by compiling the offending source code
file in mednafen which currently fails to build from source [1]:

Without "-mlong-jump-table-offsets", I get:

(sid-m68k-sbuild)root@ikarus:/tmp/mednafen-0.9.39.2+dfsg/src/hw_cpu/m68k# g++-7
-c -std=c++11 m68k.cpp -o m68k.o -I../../../include -DMSB_FIRST
-DSIZEOF_DOUBLE=8
(...)
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x22374) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
/tmp/cclNgtni.s:2848: Error: Adjusted signed .word (0x2237a) overflows:
`switch'-statement too large.
(sid-m68k-sbuild)root@ikarus:/tmp/mednafen-0.9.39.2+dfsg/src/hw_cpu/m68k#
With the option set, it works:

(sid-m68k-sbuild)root@ikarus:/tmp/mednafen-0.9.39.2+dfsg/src/hw_cpu/m68k# g++-7
-c -std=c++11 m68k.cpp -mlong-jump-table-offsets -o m68k.o -I../../../include
-DMSB_FIRST -DSIZEOF_DOUBLE=8
(sid-m68k-sbuild)root@ikarus:/tmp/mednafen-0.9.39.2+dfsg/src/hw_cpu/m68k# ls -l
total 4100
-rw-r--r-- 1 glaubitz glaubitz   21029 Jul 31 19:22 gen.cpp
-rw-r--r-- 1 glaubitz glaubitz   41754 Aug 23 05:26 m68k.cpp
-rw-r--r-- 1 glaubitz glaubitz   11780 Aug 22 22:13 m68k.h
-rw-r--r-- 1 root root 3242396 Jan  6 10:18 m68k.o
-rw-r--r-- 1 glaubitz glaubitz  868437 Jul 31 19:22 m68k_instr.inc
(sid-m68k-sbuild)root@ikarus:/tmp/mednafen-0.9.39.2+dfsg/src/hw_cpu/m68k#

Anything that speaks against applying this patch against gcc-7 and gcc-6?

> [1] 
> https://buildd.debian.org/status/fetch.php?pkg=mednafen=m68k=0.9.39.2%2Bdfsg-1=1482769622

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #10 from John Paul Adrian Glaubitz  ---
(In reply to John Paul Adrian Glaubitz from comment #9)
> Sounds good. I can give it a try in the following days or weeks and see if I
> can get a C code with such large switch statements compiled.

Building a patched version of gcc-7 now. Then I'll try whether the problem
vanishes when using the -mlong-jump-table-offsets option.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #9 from John Paul Adrian Glaubitz  ---
(In reply to Mikael Pettersson from comment #8)
> Created attachment 40362 [details]
> patch adding -mlong-jump-table-offsets option for m68k
> 
> This is the crude patch I mentioned in an older comment, forward-ported to
> trunk.  It's only a compile-time option for using long offsets, plus needed
> adjustments here and there.

Sounds good. I can give it a try in the following days or weeks and see if I
can get a C code with such large switch statements compiled.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[mikpelinux at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #8 from Mikael Pettersson  ---
Created attachment 40362
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40362=edit
patch adding -mlong-jump-table-offsets option for m68k

This is the crude patch I mentioned in an older comment, forward-ported to
trunk.  It's only a compile-time option for using long offsets, plus needed
adjustments here and there.

I haven't tried to implement CASE_VECTOR_SHORTEN_MODE since I don't think it's
useful without per-insn length attributes.

Why does the backend have three identical definitions of ASM_RETURN_CASE_JUMP?

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #7 from John Paul Adrian Glaubitz  ---
Hi!

Are there any news on this issue? We're seeing this problem in both LibreOffice
[1] (yes, *that* LibreOffice) and Mednafen [2].

Adrian

> [1] 
> https://buildd.debian.org/status/fetch.php?pkg=libreoffice=m68k=1%3A5.3.0%7Ebeta2-1=1481545623
> [2] 
> https://buildd.debian.org/status/fetch.php?pkg=mednafen=m68k=0.9.39.2%2Bdfsg-1=1475664326

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #3 from Mikael Pettersson mikpe at it dot uu.se ---
It's not too difficult to make the m68k backend use 32-bit offsets in its jump
tables (adjust CASE_VECTOR_MODE, ASM_OUTPUT_ADDR_DIFF_ELT,
ASM_RETURN_CASE_JUMP, drop the sign-extend from the tablejump expander,
likewise in the unnamed insn that matches the output from the tablejump
expander).  I have a crude patch to do that, and make it compile-time
selectable via an option.

However, it seems to me that the compiler should be able to figure out for
itself if a given jump table might need 32-bit offsets.  In the worst case
every element will overflow a (signed) 16-bit offset (0..32K-1 positive range)
and need a GAS-inserted trampoline, for a total of 2 + 6 == 8 bytes per
element.  So tables with no more than 4K elements should work with 16-bit
offsets.  Tables larger than that may have their trampolines more than 32K away
from the table PC base, which cannot work with 16-bit offsets, so for those the
compiler should use 32-bit offsets.  Seems to require implementing casesi for
m68k though.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #4 from Hans-Peter Nilsson hp at gcc dot gnu.org ---
(In reply to Mikael Pettersson from comment #3)
 It's not too difficult to make the m68k backend use 32-bit offsets in its
 jump tables (adjust CASE_VECTOR_MODE, ASM_OUTPUT_ADDR_DIFF_ELT,
 ASM_RETURN_CASE_JUMP, drop the sign-extend from the tablejump expander,
 likewise in the unnamed insn that matches the output from the tablejump
 expander).  I have a crude patch to do that,

Does your patch define CASE_VECTOR_SHORTEN_MODE?
Otherwise that'll probably be one of the first review comments.

 and make it compile-time
 selectable via an option.

FWIW, -mbigtable for sh.

 However, it seems to me that the compiler should be able to figure out for
 itself if a given jump table might need 32-bit offsets.

The length insn attribute needs to be defined in order to take advantage of the
existing machinery.  It seems it currently isn't, for m68k.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[sch...@linux-m68k.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #5 from Andreas Schwab sch...@linux-m68k.org ---
The assembler already handles jump targets that are too far away (via the
BROKEN_DOT_WORD hack), this issue is about growing the table itself too large
so that the overflow table is not reachable any more within 32K.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #6 from Hans-Peter Nilsson hp at gcc dot gnu.org ---
(In reply to Andreas Schwab from comment #5)
 The assembler already handles jump targets that are too far away (via the
 BROKEN_DOT_WORD hack), this issue is about growing the table itself too
 large so that the overflow table is not reachable any more within 32K.

There's no confusion here.  While a less involved patch will suffice where just
the table is too big, CASE_VECTOR_SHORTEN_MODE can detect and handle both
issues. I think it will also allow for jump-tables closer to the 4K entries
than the significantly lower limit you'll find workable with a less involved
patch and relying on BROKEN_DOT_WORD.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[sch...@linux-m68k.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #1 from Andreas Schwab sch...@linux-m68k.org ---
Apparently GAS has lost its BROKEN_DOT_WORD handing.

[Bug target/57583] large switches with jump tables are horribly broken on m68k

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57583

--- Comment #2 from Mikael Pettersson mikpe at it dot uu.se ---
http://sourceware.org/bugzilla/show_bug.cgi?id=15602 is the corresponding
binutils/gas bug.