[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[ubizjak at gmail dot com]

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

--- Comment #4 from Uros Bizjak <ubizjak at gmail dot com> 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.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu.org]

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

--- Comment #5 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 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.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[ubizjak at gmail dot com]

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

--- Comment #6 from Uros Bizjak <ubizjak at gmail dot com> 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?

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu.org]

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

--- Comment #7 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 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.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu.org]

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

--- Comment #8 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 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

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[uros at gcc dot gnu.org]

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

--- Comment #9 from uros at gcc dot gnu.org 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

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu.org]

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

Kazumoto Kojima <kkojima at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #10 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-11-07 23:23:24 UTC ---
Fixed on trunk.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[ubizjak at gmail dot com]

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

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-p
                   |                            |atches/2012-11/msg00451.htm
                   |                            |l
   Target Milestone|---                         |4.8.0
      Known to fail|4.8.0                       |

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[ubizjak at gmail dot com]

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

--- Comment #11 from Uros Bizjak <ubizjak at gmail dot com> 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--

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu.org]

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

--- Comment #12 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 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.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |olegendo at gcc dot gnu.org

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> 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.

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[uros at gcc dot gnu.org]

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

--- Comment #14 from uros at gcc dot gnu.org 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

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu dot org]

------- Comment #3 from kkojima at gcc dot gnu dot org  2009-11-17 22:24 -------
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.


-- 


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

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[iwamatsu at nigauri dot org]

------- Comment #2 from iwamatsu at nigauri dot org  2009-11-17 02:55 -------
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.


-- 

iwamatsu at nigauri dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iwamatsu at nigauri dot org


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

[Bug target/41993] [sh] ICE in create_pre_exit, at mode-switching.c:399

[kkojima at gcc dot gnu dot org]

------- Comment #1 from kkojima at gcc dot gnu dot org  2009-11-11 21:59 -------
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.


-- 

kkojima at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kkojima at gcc dot gnu dot
                   |                            |org
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
           Keywords|                            |ice-on-valid-code
      Known to fail|                            |4.0.4 4.1.3 4.2.4 4.3.4
                   |                            |4.4.2 4.5.0
      Known to work|                            |3.4.6
           Priority|P3                          |P4
   Last reconfirmed|0000-00-00 00:00:00         |2009-11-11 21:59:45
               date|                            |


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