diff mbox

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

Message ID 3d33e39746ca16dcd9b533ad50f1a19e0efe853d.1450224136.git.andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Dec. 16, 2015, 12:15 a.m. UTC
The use of the arc specific predicate load_update_operand is broken,
this commit fixes the error, and in the process removes the need for
load_update_operand altogether.

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,
if the use of load_update_operand is replaced with the memory_operand
predicate.

I added a new test that exposes the issue that originally highlighted
this bug for me.  Having fixed this, I am now seeing some (but not
all) of these instructions used within the wider GCC test suite.

It would be great to add more tests targeting the specific
instructions that are not covered in the wider GCC test suite, but so
far I've been unable to come up with any usable tests.  If anyone has
advice on how to trigger the generation of specific instructions that
would be great.


gcc/ChangeLog:

	* 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.

gcc/testsuite/ChangeLog:

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

Comments

Joern Wolfgang Rennecke Dec. 17, 2015, 10:20 a.m. UTC | #1
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.

Store_update_operand has the very same problem, so it would make sense 
to fix that
in the same check-in.
FWIW, while using "memory_operand" makes for simple source code, it 
introduces
duplicated checks (and they are appropriate only because the the update 
and some of the
non-update addressing modes on the ARC are different modes of the same 
encoding).
It checks that the inside of the MEM is a valid memory address, which is 
redundant
with the pattern-provided checks that there's a plus with appropriate 
base and index/update
operand inside.
Also, by using memory_operand, you are adding a check to reject volatile 
memory operands
during most optimization passes.
Note that ARC's move_src_operand and move_dest_operand are fine with 
volatile MEMs
irrespective of the setting of volatile_ok.
Problems with volatiles can rally only be expected if there are multiple 
MEMs in a single pattern,
that might alias, arithmetic on MEM that might result from 
simplifications using a different set of
MEMs, or if the machine instructions that a pattern corresponds to are 
intrinsically unsuitable for
volatile.
Needlessly rejecting volatile MEMs will reduce optimization potential; 
this tends to be more
visible on embedded platforms than with embedded code than with typical 
workstation code.

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.
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 0a77807..705d4e9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,16 @@ 
+2015-12-09  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* 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.
+
 2015-12-10  Jeff Law  <law@redhat.com>
 
 	PR tree-optimization/68619
diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index ac181a9..ef82007 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 "memory_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 "memory_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 "memory_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 "memory_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 "memory_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 "memory_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 "memory_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 "memory_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..bdad135 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")
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index bf4d198..6ab629a 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,9 @@ 
 2015-12-09  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gcc.target/arc/load-update.c: New file.
+
+2015-12-09  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gcc.target/arc/jump-around-jump.c (rtc_set_time): Declare.
 
 2015-12-10  Jeff Law  <law@redhat.com>
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];
+    }
+}