@@ -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.
===================================================================
@@ -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;
+}
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