Bug 41993 - [sh] ICE in create_pre_exit, at mode-switching.c:399
[sh] ICE in create_pre_exit, at mode-switching.c:399
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: target
4.8.0
: P4 normal
: 4.8.0
Assigned To: Not yet assigned to anyone
http://gcc.gnu.org/ml/gcc-patches/201...
: ice-on-valid-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-09 03:17 UTC by Nobuhiro Iwamatsu
Modified: 2012-11-13 16:59 UTC (History)
5 users (show)

See Also:
Host:
Target: sh4-unknown-linux-gnu
Build:
Known to work: 3.4.6
Known to fail: 4.0.4, 4.1.3, 4.2.4, 4.3.4, 4.4.2, 4.5.0
Last reconfirmed: 2012-08-06 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nobuhiro Iwamatsu 2009-11-09 03:17:51 UTC
Hi,
When I built gnustep-base, I got the following errors.
-----
mframe.m:1250: warning: cast increases required alignment of target type
mframe.m: In function 'retframe_short':
mframe.m:863: internal compiler error: in create_pre_exit, at mode-switching.c:399
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.3/README.Bugs> for instructions.
make[3]: *** [obj/mframe.m.o] Error 1
-----

And I tested following sample program with gcc HEAD of subversion.
-----
$ cat a.c 
short retframe_short (void *rframe)
{
    __builtin_return (rframe);
}
$ tools/bin/sh4-unknown-linux-gnu-gcc -c  a.c
a.c: In function 'retframe_short':
a.c:4: internal compiler error: in create_pre_exit, at mode-switching.c:399
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.4/README.Bugs> for instructions.
$ tools/bin/sh4-unknown-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=tools/bin/sh4-unknown-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/home/yoshii/work/gcc/tools/libexec/gcc/sh4-unknown-linux-gnu/4.5.0/lto-wrapper
Target: sh4-unknown-linux-gnu
Configured with: /var/src/gcc/configure --build=x86_64-pc-linux-gnu --target=sh4-unknown-linux-gnu --prefix=/home/yoshii/work/gcc/tools --with-sysroot=/home/yoshii/work/gcc/sysroot --with-cpu=sh4a --with-multilib-list=sh4a,sh4a-nofpu --disable-libgomp --disable-libmudflap --enable-incomplete_targets --enable-libssp --enable-nls --enable-shared --enable-threads=posix --enable-symvers=gnu --enable-__cxa_atexit --enable-c99 --enable-long-long --enable-languages=c,c++
Thread model: posix
gcc version 4.5.0 20091109 (experimental) (GCC)
-----

Best regards,
  Nobuhiro
Comment 1 Kazumoto Kojima 2009-11-11 21:59:45 UTC
All 4.x sh compilers fail with similar way.  Looks only when unoptimized.
Does the patch below work for you?

--- ORIG/trunk/gcc/mode-switching.c	2009-02-21 09:26:24.000000000 +0900
+++ trunk/gcc/mode-switching.c	2009-11-11 11:03:04.000000000 +0900
@@ -325,7 +325,14 @@ create_pre_exit (int n_entities, int *en
 		    else
 		      break;
 		    if (copy_start >= FIRST_PSEUDO_REGISTER)
-		      break;
+		      {
+			if (!optimize)
+			  {
+			    last_insn = return_copy;
+			    continue;
+			  }
+			break;
+		      }
 		    copy_num
 		      = hard_regno_nregs[copy_start][GET_MODE (copy_reg)];
 


BTW, I guess that __builtin_apply/__builtin_return may be a bit obsolete.
If my memory is correct, there was an argument on the list for dropping
them from the compiler.
Comment 2 Nobuhiro Iwamatsu 2009-11-17 02:55:48 UTC
Hi, Kojima-san.

Thank you for your work and patch .
I checked this patch. Work fine with gcc-4.3.4 , gcc-4.4.2 and gcc/HEAD.

> 
> BTW, I guess that __builtin_apply/__builtin_return may be a bit obsolete.
> If my memory is correct, there was an argument on the list for dropping
> them from the compiler.
> 

I dont know this infomation. Thank you.
I will check this.
Comment 3 Kazumoto Kojima 2009-11-17 22:24:01 UTC
Thanks for testing.  On second thought, the patch in #1 doesn't
the right thing and it's too intrusive if the issue happens only
with -O0 for a bit problematic feature.
Some better idea will be needed, though it seems there is no easy
way to fix this.
Comment 4 Uroš Bizjak 2012-11-04 16:45:51 UTC
I have looked a bit into this problem, since AVX vzeroupper insertion now depends on MODE_EXIT functionality. IMO, the patch in Comment #1 is correct for all optimization levels. The reason, the problem is triggered only at -O0 is that since __builtin_return loads from the memory, gcc emits offsets to memory locations using the pseudo:

...

(insn 9 8 11 2 (set (reg:SI 0 r0)
        (mem:SI (reg/f:SI 163) [0 S4 A8])) pr41933.c:3 238 {movsi_ie}
     (nil))
(insn 11 9 12 2 (set (reg:SI 165)
        (mem/f/c:SI (plus:SI (reg/f:SI 162)
                (const_int 60 [0x3c])) [0 rframe+0 S4 A32])) pr41933.c:3 238 {movsi_ie}
     (nil))
(insn 12 11 13 2 (set (reg/f:SI 164)
        (plus:SI (reg:SI 165)
            (const_int 4 [0x4]))) pr41933.c:3 62 {*addsi3_compact}
     (nil))
(insn 13 12 10 2 (set (reg:SI 64 fr0)
        (mem:SI (reg/f:SI 164) [0 S4 A8])) pr41933.c:3 238 {movsi_ie}
     (nil))
(insn 10 13 14 2 (use (reg:SI 0 r0)) pr41933.c:3 -1
     (nil))
(insn 14 10 22 2 (use (reg:SI 64 fr0)) pr41933.c:3 -1
     (nil))
(insn 22 14 0 2 (use (reg/i:SI 0 r0)) pr41933.c:4 -1
     (nil))

This additional pseudo is what breaks the compilation. At -O2, we enter mode-switching with:

(note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 2 4 3 2 (set (reg/v/f:SI 161 [ rframe ])
        (reg:SI 4 r4 [ rframe ])) pr41933.c:2 238 {movsi_ie}
     (expr_list:REG_DEAD (reg:SI 4 r4 [ rframe ])
        (nil)))
(note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
(insn 6 3 8 2 (set (reg:SI 0 r0)
        (mem:SI (reg/v/f:SI 161 [ rframe ]) [0 S4 A8])) pr41933.c:3 238 {movsi_ie}
     (nil))
(insn 8 6 7 2 (set (reg:SI 64 fr0)
        (mem:SI (plus:SI (reg/v/f:SI 161 [ rframe ])
                (const_int 4 [0x4])) [0 S4 A8])) pr41933.c:3 238 {movsi_ie}
     (expr_list:REG_DEAD (reg/v/f:SI 161 [ rframe ])
        (nil)))
(insn 7 8 9 2 (use (reg:SI 0 r0)) pr41933.c:3 -1
     (nil))
(insn 9 7 17 2 (use (reg:SI 64 fr0)) pr41933.c:3 -1
     (expr_list:REG_DEAD (reg:SI 64 fr0)
        (nil)))
(insn 17 9 0 2 (use (reg/i:SI 0 r0)) pr41933.c:4 -1
     (nil))

In this case, we found many return registers (due to __builtin_return), and consequently lowered nregs to zero. This satisfies the following assert in (!nregs) and (nregs != hard_regno_nregs[ret_start][GET_MODE (ret_reg)]) cases.

In -O0 case, we broke discovery loop too early, so we can't find all return regs. I would argue, that we should ignore non-relevant pseudos with:

--cut here--
Index: mode-switching.c
===================================================================
--- mode-switching.c    (revision 193133)
+++ mode-switching.c    (working copy)
@@ -324,7 +324,10 @@ create_pre_exit (int n_entities, int *entity_map,
                    else
                      break;
                    if (copy_start >= FIRST_PSEUDO_REGISTER)
-                     break;
+                     {
+                       last_insn = return_copy;
+                       continue;
+                     }
                    copy_num
                      = hard_regno_nregs[copy_start][GET_MODE (copy_reg)];
 
--cut here--

In the same way as in case of i.e. UNSPEC_VOLATILE in the preceeding code.
Comment 5 Kazumoto Kojima 2012-11-05 09:13:57 UTC
(In reply to comment #4)
> In -O0 case, we broke discovery loop too early, so we can't find all return
> regs. I would argue, that we should ignore non-relevant pseudos with:

You are right.  I was wrong about what is going on that case.
Comment 6 Uroš Bizjak 2012-11-05 09:16:52 UTC
(In reply to comment #5)
> > In -O0 case, we broke discovery loop too early, so we can't find all return
> > regs. I would argue, that we should ignore non-relevant pseudos with:
> 
> You are right.  I was wrong about what is going on that case.

Will you submit the patch to gcc-patches, please?
Comment 7 Kazumoto Kojima 2012-11-05 10:19:16 UTC
(In reply to comment #6)
> Will you submit the patch to gcc-patches, please?

OK, I'll send it to the list when the tests on i686-linux and sh
are done successfully.
Comment 8 Kazumoto Kojima 2012-11-06 09:16:41 UTC
Author: kkojima
Date: Tue Nov  6 09:16:34 2012
New Revision: 193210

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193210
Log:
	PR target/41993
	* mode-switching.c (create_pre_exit): Set return_copy to
	last_insn when copy_start is a pseudo reg.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/mode-switching.c
Comment 9 uros 2012-11-06 14:31:00 UTC
Author: uros
Date: Tue Nov  6 14:30:52 2012
New Revision: 193242

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193242
Log:
	PR middle-end/41993
	* gcc.dg/torture/pr41993.c: New test.


Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr41993.c
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 10 Kazumoto Kojima 2012-11-07 23:23:24 UTC
Fixed on trunk.
Comment 11 Uroš Bizjak 2012-11-11 22:45:18 UTC
(In reply to comment #10)
> Fixed on trunk.

Actually, implementing post-reload pass it looks to me, that in this place we are only interested in function return hard registers. In pre-reload mode-switching pass, there is no possibility for others, and in post-reload pass, we got bitten by the same issue as in comment #4 (again with -O0), but with hard registers.

Kaz, can you please test following patch, if it works ok SH4?

--cut here--
Index: mode-switching.c
===================================================================
--- mode-switching.c    (revision 193407)
+++ mode-switching.c    (working copy)
@@ -330,7 +330,7 @@
                          short_block = 1;
                        break;
                      }
-                   if (copy_start >= FIRST_PSEUDO_REGISTER)
+                   if (!targetm.calls.function_value_regno_p (copy_start))
                      {
                        last_insn = return_copy;
                        continue;
--cut here--
Comment 12 Kazumoto Kojima 2012-11-12 10:35:12 UTC
(In reply to comment #11)
> Kaz, can you please test following patch, if it works ok SH4?

Works fine.  I've confirmed that there are no new failures on
sh4-unknown-linux-gnu.
Comment 13 Oleg Endo 2012-11-12 20:20:26 UTC
I've also tested this on rev 193423 with
make -k check RUNTESTFLAGS="--target_board=sh-sim\{-m2a/-mb,-m2a-single/-mb,-m4/-ml,-m4-single/-ml,-m4/-mb,-m4-single/-mb}"

and no new failures.
Comment 14 uros 2012-11-13 16:59:44 UTC
Author: uros
Date: Tue Nov 13 16:59:37 2012
New Revision: 193480

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193480
Log:
	PR target/41993
	* mode-switching.c (create_pre_exit): Set return_copy to last_insn
	when copy_start is a function return regno instead of pseudo.
	Skip debug instructions in instruction scan loop.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/mode-switching.c