[08/31] jump: Also handle jumps wrapped in UNSPEC or UNSPEC_VOLATILE

Message ID alpine.LFD.2.21.2011200244300.656242@eddie.linux-mips.org
State Superseded
Headers show
Series
  • VAX: Bring the port up to date (yes, MODE_CC conversion is included)
Related show

Commit Message

Maciej W. Rozycki Nov. 20, 2020, 3:34 a.m.
VAX has interlocked branch instructions used for atomic operations and
we want to have them wrapped in UNSPEC_VOLATILE so as not to have code
carried across.  This however breaks with jump optimization and leads
to an ICE in the build of libbacktrace like:

.../libbacktrace/mmap.c:190:1: internal compiler error: in fixup_reorder_chain, at cfgrtl.c:3934
  190 | }
      | ^
0x1087d46b fixup_reorder_chain
	.../gcc/cfgrtl.c:3934
0x1087f29f cfg_layout_finalize()
	.../gcc/cfgrtl.c:4447
0x1087c74f execute
	.../gcc/cfgrtl.c:3662

on RTL like:

(jump_insn 18 17 150 4 (unspec_volatile [
            (set (pc)
                (if_then_else (eq (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1  S4 A32])
                            (const_int 1 [0x1])
                            (const_int 0 [0]))
                        (const_int 1 [0x1]))
                    (label_ref 20)
                    (pc)))
            (set (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1  S4 A32])
                    (const_int 1 [0x1])
                    (const_int 0 [0]))
                (const_int 1 [0x1]))
        ] 101) ".../libbacktrace/mmap.c":135:14 158 {jbbssisi}
     (nil)
 -> 20)

when those branches are enabled with a follow-up change.  Also showing
with:

FAIL: gcc.dg/pr61756.c (internal compiler error)

Handle branches wrapped in UNSPEC_VOLATILE then and, for consistency,
also in UNSPEC.  The presence of UNSPEC_VOLATILE will prevent such
branches from being removed as they won't be accepted by `onlyjump_p',
we just need to let them through.

	gcc/
	* jump.c (pc_set): Also accept a jump wrapped in UNSPEC or
	UNSPEC_VOLATILE.
	(any_uncondjump_p, any_condjump_p): Update comment accordingly.
---
 gcc/jump.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.11.0

Comments

Jonathan Wakely via Gcc-patches Nov. 21, 2020, 4:25 a.m. | #1
On 11/19/20 8:34 PM, Maciej W. Rozycki wrote:
> VAX has interlocked branch instructions used for atomic operations and

> we want to have them wrapped in UNSPEC_VOLATILE so as not to have code

> carried across.  This however breaks with jump optimization and leads

> to an ICE in the build of libbacktrace like:

>

> .../libbacktrace/mmap.c:190:1: internal compiler error: in fixup_reorder_chain, at cfgrtl.c:3934

>   190 | }

>       | ^

> 0x1087d46b fixup_reorder_chain

> 	.../gcc/cfgrtl.c:3934

> 0x1087f29f cfg_layout_finalize()

> 	.../gcc/cfgrtl.c:4447

> 0x1087c74f execute

> 	.../gcc/cfgrtl.c:3662

>

> on RTL like:

>

> (jump_insn 18 17 150 4 (unspec_volatile [

>             (set (pc)

>                 (if_then_else (eq (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1  S4 A32])

>                             (const_int 1 [0x1])

>                             (const_int 0 [0]))

>                         (const_int 1 [0x1]))

>                     (label_ref 20)

>                     (pc)))

>             (set (zero_extract:SI (mem/v:SI (reg/f:SI 23 [ _2 ]) [-1  S4 A32])

>                     (const_int 1 [0x1])

>                     (const_int 0 [0]))

>                 (const_int 1 [0x1]))

>         ] 101) ".../libbacktrace/mmap.c":135:14 158 {jbbssisi}

>      (nil)

>  -> 20)

>

> when those branches are enabled with a follow-up change.  Also showing

> with:

>

> FAIL: gcc.dg/pr61756.c (internal compiler error)

>

> Handle branches wrapped in UNSPEC_VOLATILE then and, for consistency,

> also in UNSPEC.  The presence of UNSPEC_VOLATILE will prevent such

> branches from being removed as they won't be accepted by `onlyjump_p',

> we just need to let them through.

>

> 	gcc/

> 	* jump.c (pc_set): Also accept a jump wrapped in UNSPEC or

> 	UNSPEC_VOLATILE.

> 	(any_uncondjump_p, any_condjump_p): Update comment accordingly.

I've got some concerns that there may be users of pc_set where handling
UNSPECs would be undesirable.  For example the uses in cfgcleanup.

Would it make sense to have the handling of UNSPECs be conditional so
that we don't get unexpected jump threading, cross jumping, etc?

jeff

Patch

diff --git a/gcc/jump.c b/gcc/jump.c
index 34a8f209e20..f4c735540f0 100644
--- a/gcc/jump.c
+++ b/gcc/jump.c
@@ -850,9 +850,17 @@  pc_set (const rtx_insn *insn)
   pat = PATTERN (insn);
 
   /* The set is allowed to appear either as the insn pattern or
-     the first set in a PARALLEL.  */
-  if (GET_CODE (pat) == PARALLEL)
-    pat = XVECEXP (pat, 0, 0);
+     the first set in a PARALLEL, UNSPEC or UNSPEC_VOLATILE.  */
+  switch (GET_CODE (pat))
+    {
+    case PARALLEL:
+    case UNSPEC:
+    case UNSPEC_VOLATILE:
+      pat = XVECEXP (pat, 0, 0);
+      break;
+    default:
+      break;
+    }
   if (GET_CODE (pat) == SET && GET_CODE (SET_DEST (pat)) == PC)
     return pat;
 
@@ -860,7 +868,7 @@  pc_set (const rtx_insn *insn)
 }
 
 /* Return true when insn is an unconditional direct jump,
-   possibly bundled inside a PARALLEL.  */
+   possibly bundled inside a PARALLEL, UNSPEC or UNSPEC_VOLATILE.  */
 
 int
 any_uncondjump_p (const rtx_insn *insn)
@@ -876,9 +884,9 @@  any_uncondjump_p (const rtx_insn *insn)
 }
 
 /* Return true when insn is a conditional jump.  This function works for
-   instructions containing PC sets in PARALLELs.  The instruction may have
-   various other effects so before removing the jump you must verify
-   onlyjump_p.
+   instructions containing PC sets in PARALLELs, UNSPECs or UNSPEC_VOLATILEs.
+   The instruction may have various other effects so before removing the jump
+   you must verify onlyjump_p.
 
    Note that unlike condjump_p it returns false for unconditional jumps.  */