diff mbox

[2/4] gcc/arc: Remove load_update_operand predicate

Message ID 20151218125229.GB21941@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Dec. 18, 2015, 12:52 p.m. UTC 
* Joern Wolfgang Rennecke <gnu@amylaar.uk> [2015-12-17 10:20:44 +0000]:

> On 16/12/15 00:15, Andrew Burgess wrote:
> >
> >	* config/arc/arc.md (*loadqi_update): Use 'memory_operand' and fix
> >	RTL pattern to include the plus.
> >	(*load_zeroextendqisi_update): Likewise.
> >	(*load_signextendqisi_update): Likewise.
> >	(*loadhi_update): Likewise.
> >	(*load_zeroextendhisi_update): Likewise.
> >	(*load_signextendhisi_update): Likewise.
> >	(*loadsi_update): Likewise.
> >	(*loadsf_update): Likewise.
> >	* config/arc/predicates.md (load_update_operand): Delete.
> 
> 
> Less reliance on this addressing modes orthogonality, no change of volatile
> behaviour,
> and maybe a slightly faster compiler, would be to just have an
> "update_operand" or
> "any_mem_operand" predicate checking that the inside is a MEM. and leave the
> address
> processing entirely to the instruction pattern and its operand predicates.

A revised patch is below, updated to use a new any_mem_operand
predicate.  I've updated store_update_operand patch in the same say,
and will send that as a follow-up to to the 3/4 email.

Thanks,
Andrew

--

The current use of the arc specific predicate load_update_operand is
broken, this commit fixes the error, and in the process moves to a more
generic arc specific predicate any_mem_operand.

Currently load_update_operand is used with match_operator, the
load_update_operand checks that the operand is a MEM operand, with an
operand that is a plus, the plus in turn has operands that are a
register and an immediate.

However, the match_operator already checks the structure of the rtl
tree, only in this case a different rtl pattern is checked for, in this
case the operand must have two child operands, one a register operand
and one an immediate operand.

The mistake here is that the plus part of the rtl tree has been missed
from the define_insn rtl pattern.  The consequence of this mistake is
that a MEM operand will match the load_update_operand predicate, then
the second operand of the MEM insn will then be passed to the
nonmemory_operand predicate, which assumes it will be passed an
rtl_insn.  However, the second operand of a MEM insn is the alias set
for the address, not an rtl_insn.

When fixing the rtl pattern within the define_insn it becomes obvious
that all of the checks currently contained within the
load_update_operand predicate are now contains within the rtl pattern,
the only thing that a predicate now needs to do is to confirm that the
operand being matched by the match_operator is a memory operand.  This
is what the new predicate 'any_mem_operand' does.

The reasons for creating a new predicate 'any_mem_operand' rather than
using the common 'memory_operand' predicate are described in this
mailing list post:
  https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01729.html

gcc/ChangeLog:

	* config/arc/arc.md (*loadqi_update): Use new 'any_mem_operand'
	and fix RTL pattern to include the plus.
	(*load_zeroextendqisi_update): Likewise.
	(*load_signextendqisi_update): Likewise.
	(*loadhi_update): Likewise.
	(*load_zeroextendhisi_update): Likewise.
	(*load_signextendhisi_update): Likewise.
	(*loadsi_update): Likewise.
	(*loadsf_update): Likewise.
	* config/arc/predicates.md (load_update_operand): Delete.
	(any_mem_operand): New predicate.

gcc/testsuite/ChangeLog:

	* gcc.target/arc/load-update.c: New file.
---
 gcc/config/arc/arc.md                      | 48 +++++++++++++++---------------
 gcc/config/arc/predicates.md               | 21 ++-----------
 gcc/testsuite/gcc.target/arc/load-update.c | 20 +++++++++++++
 5 files changed, 65 insertions(+), 42 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/arc/load-update.c

Comments

Joern Wolfgang Rennecke Dec. 19, 2015, 7:34 p.m. UTC | #1 
On 18/12/15 12:52, Andrew Burgess wrote:
> gcc/ChangeLog:
>
> 	* config/arc/arc.md (*loadqi_update): Use new 'any_mem_operand'
> 	and fix RTL pattern to include the plus.
> 	(*load_zeroextendqisi_update): Likewise.
> 	(*load_signextendqisi_update): Likewise.
> 	(*loadhi_update): Likewise.
> 	(*load_zeroextendhisi_update): Likewise.
> 	(*load_signextendhisi_update): Likewise.
> 	(*loadsi_update): Likewise.
> 	(*loadsf_update): Likewise.
> 	* config/arc/predicates.md (load_update_operand): Delete.
> 	(any_mem_operand): New predicate.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/arc/load-update.c: New file.
Thanks.  I have applied this patch.
diff mbox

Patch

diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index ac181a9..7ca4431 100644
--- a/gcc/config/arc/arc.md
+++ b/gcc/config/arc/arc.md
@@ -1114,9 +1114,9 @@ 
 ;; Note: loadqi_update has no 16-bit variant
 (define_insn "*loadqi_update"
   [(set (match_operand:QI 3 "dest_reg_operand" "=r,r")
-	(match_operator:QI 4 "load_update_operand"
-	 [(match_operand:SI 1 "register_operand" "0,0")
-	  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")]))
+        (match_operator:QI 4 "any_mem_operand"
+         [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+                   (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))]))
    (set (match_operand:SI 0 "dest_reg_operand" "=r,r")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1126,9 +1126,9 @@ 
 
 (define_insn "*load_zeroextendqisi_update"
   [(set (match_operand:SI 3 "dest_reg_operand" "=r,r")
-	(zero_extend:SI (match_operator:QI 4 "load_update_operand"
-			 [(match_operand:SI 1 "register_operand" "0,0")
-			  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")])))
+	(zero_extend:SI (match_operator:QI 4 "any_mem_operand"
+			 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+			           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))])))
    (set (match_operand:SI 0 "dest_reg_operand" "=r,r")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1138,9 +1138,9 @@ 
 
 (define_insn "*load_signextendqisi_update"
   [(set (match_operand:SI 3 "dest_reg_operand" "=r,r")
-	(sign_extend:SI (match_operator:QI 4 "load_update_operand"
-			 [(match_operand:SI 1 "register_operand" "0,0")
-			  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")])))
+	(sign_extend:SI (match_operator:QI 4 "any_mem_operand"
+			 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+			           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))])))
    (set (match_operand:SI 0 "dest_reg_operand" "=r,r")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1164,9 +1164,9 @@ 
 ;; Note: no 16-bit variant for this pattern
 (define_insn "*loadhi_update"
   [(set (match_operand:HI 3 "dest_reg_operand" "=r,r")
-	(match_operator:HI 4 "load_update_operand"
-	 [(match_operand:SI 1 "register_operand" "0,0")
-	  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")]))
+	(match_operator:HI 4 "any_mem_operand"
+	 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+	           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))]))
    (set (match_operand:SI 0 "dest_reg_operand" "=w,w")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1176,9 +1176,9 @@ 
 
 (define_insn "*load_zeroextendhisi_update"
   [(set (match_operand:SI 3 "dest_reg_operand" "=r,r")
-	(zero_extend:SI (match_operator:HI 4 "load_update_operand"
-			 [(match_operand:SI 1 "register_operand" "0,0")
-			  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")])))
+	(zero_extend:SI (match_operator:HI 4 "any_mem_operand"
+			 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+			           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))])))
    (set (match_operand:SI 0 "dest_reg_operand" "=r,r")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1189,9 +1189,9 @@ 
 ;; Note: no 16-bit variant for this instruction
 (define_insn "*load_signextendhisi_update"
   [(set (match_operand:SI 3 "dest_reg_operand" "=r,r")
-	(sign_extend:SI (match_operator:HI 4 "load_update_operand"
-			 [(match_operand:SI 1 "register_operand" "0,0")
-			  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")])))
+	(sign_extend:SI (match_operator:HI 4 "any_mem_operand"
+			 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+			           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))])))
    (set (match_operand:SI 0 "dest_reg_operand" "=w,w")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1214,9 +1214,9 @@ 
 ;; No 16-bit variant for this instruction pattern
 (define_insn "*loadsi_update"
   [(set (match_operand:SI 3 "dest_reg_operand" "=r,r")
-	(match_operator:SI 4 "load_update_operand"
-	 [(match_operand:SI 1 "register_operand" "0,0")
-	  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")]))
+	(match_operator:SI 4 "any_mem_operand"
+	 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+	           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))]))
    (set (match_operand:SI 0 "dest_reg_operand" "=w,w")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
@@ -1238,9 +1238,9 @@ 
 
 (define_insn "*loadsf_update"
   [(set (match_operand:SF 3 "dest_reg_operand" "=r,r")
-	(match_operator:SF 4 "load_update_operand"
-	 [(match_operand:SI 1 "register_operand" "0,0")
-	  (match_operand:SI 2 "nonmemory_operand" "rI,Cal")]))
+	(match_operator:SF 4 "any_mem_operand"
+	 [(plus:SI (match_operand:SI 1 "register_operand" "0,0")
+	           (match_operand:SI 2 "nonmemory_operand" "rI,Cal"))]))
    (set (match_operand:SI 0 "dest_reg_operand" "=w,w")
 	(plus:SI (match_dup 1) (match_dup 2)))]
   ""
diff --git a/gcc/config/arc/predicates.md b/gcc/config/arc/predicates.md
index de0735a..268ff7e 100644
--- a/gcc/config/arc/predicates.md
+++ b/gcc/config/arc/predicates.md
@@ -460,24 +460,6 @@ 
 }
 )
 
-;; Return true if OP is valid load with update operand.
-(define_predicate "load_update_operand"
-  (match_code "mem")
-{
-  if (GET_CODE (op) != MEM
-      || GET_MODE (op) != mode)
-    return 0;
-  op = XEXP (op, 0);
-  if (GET_CODE (op) != PLUS
-      || GET_MODE (op) != Pmode
-      || !register_operand (XEXP (op, 0), Pmode)
-      || !nonmemory_operand (XEXP (op, 1), Pmode))
-    return 0;
-  return 1;
-
-}
-)
-
 ;; Return true if OP is valid store with update operand.
 (define_predicate "store_update_operand"
   (match_code "mem")
@@ -817,3 +799,6 @@ 
 (define_predicate "mem_noofs_operand"
   (and (match_code "mem")
        (match_code "reg" "0")))
+
+(define_predicate "any_mem_operand"
+  (match_code "mem"))
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/arc/load-update.c b/gcc/testsuite/gcc.target/arc/load-update.c
new file mode 100644
index 0000000..8299cb7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arc/load-update.c
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Os" } */
+
+/* This caused a segfault due to incorrect rtl pattern in some
+   instructions.  */
+
+int a, d;
+char *b;
+
+void fn1()
+{
+  char *e = 0;
+  for (; d; ++a)
+    {
+      char c = b [0];
+      *e++ = '.';
+      *e++ = 4;
+      *e++ = "0123456789abcdef" [c & 5];
+    }
+}