Patchwork Committed: fix PR target/53120, constraint modifier "+" on operand tied by matching-constraint, "0".

login
register
mail settings
Submitter Hans-Peter Nilsson
Date April 26, 2012, 12:36 a.m.
Message ID <201204260036.q3Q0aKE7003162@ignucius.se.axis.com>
Download mbox | patch
Permalink /patch/155146/
State New
Headers show

Comments

Hans-Peter Nilsson - April 26, 2012, 12:36 a.m.
The subject line points out the bug.  This combination of
constraints is invalid, and almost no other target has it (and
none together with strict_low_part): you can't reload a
read/write operand (one with a "+" modifier, such as a
strict_low_part destination) for which there is an input-only
matching operand (typically "0" for the input-operand, but
vax/builtins.md has them the other way round).  Think about it.
You could cook something up for reload to handle a
strict_low_part destination (as mentioned in the PR), but I
doubt there's much value in that, and I believe reload hackers
are more likely to remove strict_low_part altogether.  As usual
with such bugs, it will work until there's code where such an
insn has to be reloaded.  And as mentioned in the FIXME, this
could very well be checked as part of the RTL-reader sanity
checks.

CC to m32c and vax maintainers as I spot a case of this in
m32c/bitops.md and several in vax/builtins.md.

As the comment mentions, a similar and-with-strict_low_part-
destination insn in m68k.md has a comment pointing out the
issue; it and the strict_low_part match_dups to which it refers
have been there since (artificial) svn revision 372, ten
(artificial) revisions from the introduction of reload.c(!)

I took the opportunity to remove the use of constraint "O" from
the affected insns; it's an artefact from the time before all
const_int's in insns were trunc_int_for_mode'd for the mode
they're used; that constraint can't match anymore, matching only
integers outside the QI and HI range (for HImode use, dominated
by "L" to not match 255-31..255).  This cascades to the other
const_int-matching constraints: they're redundant with the "g"
presence.  And yes, a constraint for -32..-1 to match an operand
with a "quick" integer size might be an improvement, but I
believe it's small enough that I won't bother until I see code
where it'd help (and can add a test-case as a regression test).
Same change installed on 4.7 only for ease of maintaintenance.
Sorry to see the three-operand alternative (the one without the
matching-constraint) having to go, but it's not like
strict_low_part constructs are common enough to matter (QED:
this bug being dormant for so long).  And finally, yes, I could
probably combine these similar different-ISA-insns by adding and
using the "enabled" attribute on the alternatives.  Later, maybe.

Tested x cris-elf, committed trunk and 4.7 branches.

gcc:
	PR target/53120
	* config/cris/cris.md ("*andhi_lowpart_v32")
	("*andqi_lowpart_v32"): Change first input-only operand from
	a (match_operand ...) to (match_dup 0).  Drop alternatives with
	const_int-matching constraints for redundancy.
	("*andhi_lowpart_non_v32", "*andqi_lowpart_non_v32"): Ditto.  Drop
	three-operand alternative.

gcc/testsuite:
	PR target/53120
	* gcc.dg/torture/pr53120.c: New test.


brgds, H-P

Patch

diff --git gcc/config/cris/cris.md gcc/config/cris/cris.md
index 95eddab..308a2eb 100644
--- gcc/config/cris/cris.md
+++ gcc/config/cris/cris.md
@@ -3031,36 +3031,51 @@ 
 
 ;; A strict_low_part pattern.
 
+;; Note the use of (match_dup 0) for the first operand of the operation
+;; here.  Reload can't handle an operand pair where one is read-write
+;; and must match a read, like in:
+;; (insn 80 79 81 4
+;;  (set (strict_low_part
+;;        (subreg:QI (reg/v:SI 0 r0 [orig:36 data ] [36]) 0))
+;;       (and:QI
+;;        (subreg:QI (reg:SI 15 acr [orig:27 D.7531 ] [27]) 0)
+;;        (const_int -64 [0xf..fc0]))) x.c:126 147 {*andqi_lowpart_v32}
+;;  (nil))
+;; In theory, it could reload this as a movstrictqi of the register
+;; operand at the and:QI to the destination register and change the
+;; and:QI operand to the same as the read-write output operand and the
+;; result would be recognized, but it doesn't recognize that's a valid
+;; reload for a strict_low_part-destination; it just sees a "+" at the
+;; destination constraints.  Better than adding complexity to reload is
+;; to follow the lead of m68k (see comment that begins with "These insns
+;; must use MATCH_DUP") since prehistoric times and make it just a
+;; match_dup.  FIXME: a sanity-check in gen* to refuse an insn with
+;; input-constraints matching input-output-constraints, e.g. "+r" <- "0".
+
 (define_insn "*andhi_lowpart_non_v32"
   [(set (strict_low_part
-	 (match_operand:HI 0 "register_operand"	       "+r,r, r,r,r,r"))
-	(and:HI (match_operand:HI 1 "register_operand" "%0,0, 0,0,0,r")
-		(match_operand:HI 2 "general_operand"   "r,Q>,L,O,g,!To")))]
+	 (match_operand:HI 0 "register_operand"	       "+r,r,r"))
+	(and:HI (match_dup 0)
+		(match_operand:HI 1 "general_operand"   "r,Q>,g")))]
   "!TARGET_V32"
   "@
-   and.w %2,%0
-   and.w %2,%0
-   and.w %2,%0
-   anDq %b2,%0
-   and.w %2,%0
-   and.w %2,%1,%0"
-  [(set_attr "slottable" "yes,yes,no,yes,no,no")
-   (set_attr "cc" "normal,normal,normal,clobber,normal,normal")])
+   and.w %1,%0
+   and.w %1,%0
+   and.w %1,%0"
+  [(set_attr "slottable" "yes,yes,no")])
 
 (define_insn "*andhi_lowpart_v32"
   [(set (strict_low_part
-	 (match_operand:HI 0 "register_operand" "+r,r,r,r,r"))
-	(and:HI (match_operand:HI 1 "register_operand" "%0,0,0,0,0")
-		(match_operand:HI 2 "general_operand" "r,Q>,L,O,g")))]
+	 (match_operand:HI 0 "register_operand" "+r,r,r"))
+	(and:HI (match_dup 0)
+		(match_operand:HI 1 "general_operand" "r,Q>,g")))]
   "TARGET_V32"
   "@
-   and.w %2,%0
-   and.w %2,%0
-   and.w %2,%0
-   anDq %b2,%0
-   and.w %2,%0"
-  [(set_attr "slottable" "yes,yes,no,yes,no")
-   (set_attr "cc" "noov32,noov32,noov32,clobber,noov32")])
+   and.w %1,%0
+   and.w %1,%0
+   and.w %1,%0"
+  [(set_attr "slottable" "yes,yes,no")
+   (set_attr "cc" "noov32")])
 
 (define_expand "andqi3"
   [(set (match_operand:QI 0 "register_operand")
@@ -3100,32 +3115,28 @@ 
 
 (define_insn "*andqi_lowpart_non_v32"
   [(set (strict_low_part
-	 (match_operand:QI 0 "register_operand"	       "+r,r, r,r,r"))
-	(and:QI (match_operand:QI 1 "register_operand" "%0,0, 0,0,r")
-		(match_operand:QI 2 "general_operand"   "r,Q>,O,g,!To")))]
+	 (match_operand:QI 0 "register_operand"	       "+r,r,r"))
+	(and:QI (match_dup 0)
+		(match_operand:QI 1 "general_operand"   "r,Q>,g")))]
   "!TARGET_V32"
   "@
-   and.b %2,%0
-   and.b %2,%0
-   andQ %b2,%0
-   and.b %2,%0
-   and.b %2,%1,%0"
-  [(set_attr "slottable" "yes,yes,yes,no,no")
-   (set_attr "cc" "normal,normal,clobber,normal,normal")])
+   and.b %1,%0
+   and.b %1,%0
+   and.b %1,%0"
+  [(set_attr "slottable" "yes,yes,no")])
 
 (define_insn "*andqi_lowpart_v32"
   [(set (strict_low_part
-	 (match_operand:QI 0 "register_operand" "+r,r,r,r"))
-	(and:QI (match_operand:QI 1 "register_operand" "%0,0,0,0")
-		(match_operand:QI 2 "general_operand" "r,Q>,O,g")))]
+	 (match_operand:QI 0 "register_operand" "+r,r,r"))
+	(and:QI (match_dup 0)
+		(match_operand:QI 1 "general_operand" "r,Q>,g")))]
   "TARGET_V32"
   "@
-   and.b %2,%0
-   and.b %2,%0
-   andQ %b2,%0
-   and.b %2,%0"
-  [(set_attr "slottable" "yes,yes,yes,no")
-   (set_attr "cc" "noov32,noov32,clobber,noov32")])
+   and.b %1,%0
+   and.b %1,%0
+   and.b %1,%0"
+  [(set_attr "slottable" "yes,yes,no")
+   (set_attr "cc" "noov32")])
 
 ;; Bitwise or.
 
Index: gcc/testsuite/gcc.dg/torture/pr53120.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr53120.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr53120.c	(revision 0)
@@ -0,0 +1,110 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fno-tree-sra" } */
+typedef struct {
+  unsigned int en : 1;
+  unsigned int bit_order : 1;
+  unsigned int scl_io : 1;
+  unsigned int scl_inv : 1;
+  unsigned int sda0_io : 1;
+  unsigned int sda0_idle : 1;
+  unsigned int sda1_io : 1;
+  unsigned int sda1_idle : 1;
+  unsigned int sda2_io : 1;
+  unsigned int sda2_idle : 1;
+  unsigned int sda3_io : 1;
+  unsigned int sda3_idle : 1;
+  unsigned int sda_sel : 2;
+  unsigned int sen_idle : 1;
+  unsigned int sen_inv : 1;
+  unsigned int sen_sel : 2;
+  unsigned int dummy1 : 14;
+} reg_gio_rw_i2c1_cfg;
+
+typedef struct {
+  unsigned int data0 : 8;
+  unsigned int data1 : 8;
+  unsigned int data2 : 8;
+  unsigned int data3 : 8;
+} reg_gio_rw_i2c1_data;
+
+typedef struct {
+  unsigned int trf_bits : 6;
+  unsigned int switch_dir : 6;
+  unsigned int extra_start : 3;
+  unsigned int early_end : 1;
+  unsigned int start_stop : 1;
+  unsigned int ack_dir0 : 1;
+  unsigned int ack_dir1 : 1;
+  unsigned int ack_dir2 : 1;
+  unsigned int ack_dir3 : 1;
+  unsigned int ack_dir4 : 1;
+  unsigned int ack_dir5 : 1;
+  unsigned int ack_bit : 1;
+  unsigned int start_bit : 1;
+  unsigned int freq : 2;
+  unsigned int dummy1 : 5;
+} reg_gio_rw_i2c1_ctrl;
+
+extern reg_gio_rw_i2c1_cfg reg_gio;
+extern reg_gio_rw_i2c1_data reg_data;
+extern int reg_start;
+extern reg_gio_rw_i2c1_ctrl reg_ctrl;
+
+extern void foobar(void);
+extern void foo(int);
+extern void frob(unsigned int);
+extern void bar(int);
+extern void baz(void);
+
+unsigned int f(int *devspec, unsigned int addr)
+{
+ reg_gio_rw_i2c1_ctrl ctrl = {0};
+ reg_gio_rw_i2c1_data data = {0};
+
+ foobar();
+
+ static int first = 1;
+
+ if (first) {
+  reg_gio_rw_i2c1_cfg cfg = {0};
+  first = 0;
+
+  foo(1);
+  cfg.sda0_idle = 1;
+  cfg.sda0_io = 0;
+  cfg.scl_inv = 0;
+  cfg.scl_io = 0;
+  cfg.bit_order = 1;
+  cfg.sda_sel = 0;
+  cfg.sen_sel = 0;
+  cfg.en = 1;
+  reg_gio = cfg;
+ }
+
+ ctrl.freq = 1;
+ ctrl.start_bit = 0;
+ ctrl.ack_bit = 1;
+ ctrl.ack_dir0 = 0;
+ ctrl.ack_dir1 = 0;
+ ctrl.ack_dir2 = 0;
+ ctrl.ack_dir3 = 1;
+ ctrl.ack_dir4 = 0;
+ ctrl.ack_dir5 = 0;
+ ctrl.start_stop = 1;
+ ctrl.early_end = 0;
+ ctrl.extra_start = 2;
+ ctrl.switch_dir = 8*3;
+ ctrl.trf_bits = 8*4;
+ reg_ctrl = ctrl;
+ frob(0xac);
+ data.data0 = devspec[1] & 192;
+ data.data1 = addr;
+ data.data2 = devspec[1] | 0x01;
+ reg_data = data;
+ reg_start = 1;
+ bar(100);
+ data = reg_data;
+ baz();
+
+ return data.data3;
+}