[11/31] VAX: Correct `sync_lock_test_and_set' and `sync_lock_release' builtins

Message ID alpine.LFD.2.21.2011200247340.656242@eddie.linux-mips.org
State New
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:35 a.m.
Remove an ICE like:

during RTL pass: expand
.../libatomic/tas_n.c: In function 'libat_test_and_set_1':
.../libatomic/tas_n.c:39:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1298
   39 | }
      | ^
0x108a09ff patch_jump_insn
	.../gcc/cfgrtl.c:1298
0x108a0b07 redirect_branch_edge
	.../gcc/cfgrtl.c:1325
0x108a124b rtl_redirect_edge_and_branch
	.../gcc/cfgrtl.c:1458
0x1087f6d3 redirect_edge_and_branch(edge_def*, basic_block_def*)
	.../gcc/cfghooks.c:373
0x11d6264b try_forward_edges
	.../gcc/cfgcleanup.c:562
0x11d6b0eb try_optimize_cfg
	.../gcc/cfgcleanup.c:2960
0x11d6ba4f cleanup_cfg(int)
	.../gcc/cfgcleanup.c:3174
0x10870b3f execute
	.../gcc/cfgexpand.c:6763

triggered with an RTL pattern like:

(jump_insn 8 7 20 2 (parallel [
            (set (pc)
                (if_then_else (ne (zero_extract:SI (mem/v:QI (mem/f/c:SI (reg/f:SI 16 virtual-incoming-args) [1 mptr+0 S4 A32]) [-1  S1 A8])
                            (const_int 1 [0x1])
                            (const_int 0 [0]))
                        (const_int 0 [0]))
                    (label_ref 10)
                    (pc)))
            (set (zero_extract:SI (mem/v:QI (mem/f/c:SI (reg/f:SI 16 virtual-incoming-args) [1 mptr+0 S4 A32]) [-1  S1 A8])
                    (const_int 1 [0x1])
                    (const_int 0 [0]))
                (const_int 1 [0x1]))
        ]) ".../libatomic/tas_n.c":38:12 -1
     (nil)
 -> 10)

caused by a volatile memory reference used that is not accepted by the
`memory_operand' predicate of the `jbbssiqi' insn explicitly referred
from the `sync_lock_test_and_setqi' expander.  Also seen with:

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

Define a new `any_memory_operand' predicate accepting both ordinary and
volatile memory references and use it with the `jbb<ccss>i<mode>' insn,
so as to address the ICE.

Also remove useless operations from the `sync_lock_test_and_set<mode>'
and `sync_lock_release<mode>' expanders as those always either complete
or fail and therefore never fall through to using their template other
than to match operands.  Wrap `jbb<ccss>i<mode>' into `unspec_volatile'
instead so that the jump does not get removed or reordered.  Share one
index to avoid a complication around the iterators since the index is
nowhere referred to anyway and the pattern required pulled by its name.

Test cases will be added separately.

	gcc/
	* config/vax/predicates.md (volatile_mem_operand)
	(any_memory_operand): New predicates.
	* config/vax/builtins.md (VUNSPEC_UNLOCK): Remove constant.
	(sync_lock_test_and_set<mode>): Remove `set' and `unspec'
	operations, match operands only.  Reformat.
	(sync_lock_release<mode>): Likewise.  Remove cruft.
	(jbb<ccss>i<mode>): Wrap into `unspec_volatile', use
	`any_memory_operand' predicate.
---
 gcc/config/vax/builtins.md   | 36 +++++++++++++++++-------------------
 gcc/config/vax/predicates.md | 16 ++++++++++++++++
 2 files changed, 33 insertions(+), 19 deletions(-)

-- 
2.11.0

Comments

Martin Sebor via Gcc-patches Nov. 21, 2020, 4:26 a.m. | #1
On 11/19/20 8:35 PM, Maciej W. Rozycki wrote:
> Remove an ICE like:

>

> during RTL pass: expand

> .../libatomic/tas_n.c: In function 'libat_test_and_set_1':

> .../libatomic/tas_n.c:39:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1298

>    39 | }

>       | ^

> 0x108a09ff patch_jump_insn

> 	.../gcc/cfgrtl.c:1298

> 0x108a0b07 redirect_branch_edge

> 	.../gcc/cfgrtl.c:1325

> 0x108a124b rtl_redirect_edge_and_branch

> 	.../gcc/cfgrtl.c:1458

> 0x1087f6d3 redirect_edge_and_branch(edge_def*, basic_block_def*)

> 	.../gcc/cfghooks.c:373

> 0x11d6264b try_forward_edges

> 	.../gcc/cfgcleanup.c:562

> 0x11d6b0eb try_optimize_cfg

> 	.../gcc/cfgcleanup.c:2960

> 0x11d6ba4f cleanup_cfg(int)

> 	.../gcc/cfgcleanup.c:3174

> 0x10870b3f execute

> 	.../gcc/cfgexpand.c:6763

>

> triggered with an RTL pattern like:

>

> (jump_insn 8 7 20 2 (parallel [

>             (set (pc)

>                 (if_then_else (ne (zero_extract:SI (mem/v:QI (mem/f/c:SI (reg/f:SI 16 virtual-incoming-args) [1 mptr+0 S4 A32]) [-1  S1 A8])

>                             (const_int 1 [0x1])

>                             (const_int 0 [0]))

>                         (const_int 0 [0]))

>                     (label_ref 10)

>                     (pc)))

>             (set (zero_extract:SI (mem/v:QI (mem/f/c:SI (reg/f:SI 16 virtual-incoming-args) [1 mptr+0 S4 A32]) [-1  S1 A8])

>                     (const_int 1 [0x1])

>                     (const_int 0 [0]))

>                 (const_int 1 [0x1]))

>         ]) ".../libatomic/tas_n.c":38:12 -1

>      (nil)

>  -> 10)

>

> caused by a volatile memory reference used that is not accepted by the

> `memory_operand' predicate of the `jbbssiqi' insn explicitly referred

> from the `sync_lock_test_and_setqi' expander.  Also seen with:

>

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

>

> Define a new `any_memory_operand' predicate accepting both ordinary and

> volatile memory references and use it with the `jbb<ccss>i<mode>' insn,

> so as to address the ICE.

>

> Also remove useless operations from the `sync_lock_test_and_set<mode>'

> and `sync_lock_release<mode>' expanders as those always either complete

> or fail and therefore never fall through to using their template other

> than to match operands.  Wrap `jbb<ccss>i<mode>' into `unspec_volatile'

> instead so that the jump does not get removed or reordered.  Share one

> index to avoid a complication around the iterators since the index is

> nowhere referred to anyway and the pattern required pulled by its name.

>

> Test cases will be added separately.

>

> 	gcc/

> 	* config/vax/predicates.md (volatile_mem_operand)

> 	(any_memory_operand): New predicates.

> 	* config/vax/builtins.md (VUNSPEC_UNLOCK): Remove constant.

> 	(sync_lock_test_and_set<mode>): Remove `set' and `unspec'

> 	operations, match operands only.  Reformat.

> 	(sync_lock_release<mode>): Likewise.  Remove cruft.

> 	(jbb<ccss>i<mode>): Wrap into `unspec_volatile', use

> 	`any_memory_operand' predicate.

OK
jeff

Patch

diff --git a/gcc/config/vax/builtins.md b/gcc/config/vax/builtins.md
index 8bbcd603d13..7e27854a8b0 100644
--- a/gcc/config/vax/builtins.md
+++ b/gcc/config/vax/builtins.md
@@ -19,8 +19,7 @@ 
 
 (define_constants
   [
-    (VUNSPEC_LOCK 100)		; sync lock and test
-    (VUNSPEC_UNLOCK 101)	; sync lock release
+    (VUNSPEC_LOCK 100)		; sync lock operations
   ]
 )
 
@@ -58,10 +57,9 @@  (define_insn "ffssi2_internal"
   "ffs $0,$32,%1,%0")
 
 (define_expand "sync_lock_test_and_set<mode>"
-  [(set (match_operand:VAXint 0 "nonimmediate_operand" "=&g")
-	(unspec:VAXint [(match_operand:VAXint 1 "memory_operand" "+m")
-		    (match_operand:VAXint 2 "const_int_operand" "n")
-		   ] VUNSPEC_LOCK))]
+  [(match_operand:VAXint 0 "nonimmediate_operand" "=&g")
+   (match_operand:VAXint 1 "memory_operand" "+m")
+   (match_operand:VAXint 2 "const_int_operand" "n")]
   ""
   "
 {
@@ -72,46 +70,46 @@  (define_expand "sync_lock_test_and_set<mode>"
 
   label = gen_label_rtx ();
   emit_move_insn (operands[0], const1_rtx);
-  emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
+  emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label,
+				    operands[1]));
   emit_move_insn (operands[0], const0_rtx);
   emit_label (label);
   DONE;
 }")
 
 (define_expand "sync_lock_release<mode>"
-  [(set (match_operand:VAXint 0 "memory_operand" "+m")
-	(unspec:VAXint [(match_operand:VAXint 1 "const_int_operand" "n")
-		   ] VUNSPEC_UNLOCK))]
+  [(match_operand:VAXint 0 "memory_operand" "+m")
+   (match_operand:VAXint 1 "const_int_operand" "n")]
   ""
   "
 {
   rtx label;
+
   if (operands[1] != const0_rtx)
     FAIL;
-#if 1
+
   label = gen_label_rtx ();
-  emit_jump_insn (gen_jbbcci<mode> (operands[0], const0_rtx, label, operands[0]));
+  emit_jump_insn (gen_jbbcci<mode> (operands[0], const0_rtx, label,
+				    operands[0]));
   emit_label (label);
-#else
-  emit_move_insn (operands[0], const0_rtx);
-#endif
   DONE;
 }")
 
 (define_insn "jbb<ccss>i<mode>"
-  [(parallel
+  [(unspec_volatile
     [(set (pc)
 	  (if_then_else
 	    (eq (zero_extract:SI
-		  (match_operand:VAXint 0 "memory_operand" "<bb_mem>")
+		  (match_operand:VAXint 0 "any_memory_operand" "<bb_mem>")
 		  (const_int 1)
 		  (match_operand:SI 1 "general_operand" "nrmT"))
 		(const_int bit))
 	    (label_ref (match_operand 2 "" ""))
 	    (pc)))
-     (set (zero_extract:SI (match_operand:VAXint 3 "memory_operand" "+0")
+     (set (zero_extract:SI (match_operand:VAXint 3 "any_memory_operand" "+0")
 			   (const_int 1)
 			   (match_dup 1))
-	  (const_int bit))])]
+	  (const_int bit))]
+    VUNSPEC_LOCK)]
   ""
   "jb<ccss>i %1,%0,%l2")
diff --git a/gcc/config/vax/predicates.md b/gcc/config/vax/predicates.md
index 93e91e499a6..7c97b366604 100644
--- a/gcc/config/vax/predicates.md
+++ b/gcc/config/vax/predicates.md
@@ -93,3 +93,19 @@  (define_predicate "general_addsub_di_operand"
    (and (match_code "const_int,const_double,subreg,reg,mem")
 	(and (match_operand:DI 0 "general_operand" "")
 	     (not (match_operand:DI 0 "illegal_addsub_di_memory_operand")))))
+
+;; Return 1 if the operand is in volatile memory.  Note that during the
+;; RTL generation phase, `memory_operand' does not return TRUE for
+;; volatile memory references.  So this function allows us to recognize
+;; volatile references where it's safe.
+(define_predicate "volatile_mem_operand"
+  (and (match_code "mem")
+       (match_test "MEM_VOLATILE_P (op)")
+       (if_then_else (match_test "reload_completed")
+	 (match_operand 0 "memory_operand")
+	 (match_test "memory_address_p (mode, XEXP (op, 0))"))))
+
+;; Return 1 if the operand is a volatile or non-volatile memory operand.
+(define_predicate "any_memory_operand"
+  (ior (match_operand 0 "memory_operand")
+       (match_operand 0 "volatile_mem_operand")))