diff mbox

Remove mode argument from gen_rtx_SET

Message ID 877fsjupyh.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford May 8, 2015, 2:03 p.m. UTC 
Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> Am 2015-05-08 um 13:57 schrieb Segher Boessenkool:
>> On Fri, May 08, 2015 at 12:32:30PM +0200, Franz Sirl wrote:
>>> this patch (r222882 is fine, r222883 fails) breaks bootstrap for me on
>>> x86_64-linux-gnu:
>>
>> i386.md has "set:BND" twice; replace that with just "set", and all
>> should be fine.
>>
>> Maybe gen* should warn on this; maybe it already does.
>
> I didn't see a warning in the logs at least. But your suggestion fixes 
> the bootstrap for me.

Thanks.  I installed this as obvious after testing that x86_64-linux-gnu
built with --enable-libmpx and that rx-elf could handle:

  void f(long long *a) { a[0] = a[1]; }

when -mlra was passed.

There's also one in a comment in msp430.md:

; This pattern is identical to the truncsipsi2 pattern except
; that it uses a SUBREG instead of a TRUNC.  It is needed in
; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
; into (SET:PSI (PSI)).

I'm not sure what that's supposed to mean (what's an SI set of a PSI
subreg?), but I suspect removing the mode would lose information,
so I left it alone.

I'll follow up with a patch to make the generators raise an error
for this, as well as to restore the "missing mode" diagnostics
mentioned in the genrecog thread.

Sorry for the breakage.

Richard


gcc/
	* config/i386/i386.md (<mode>_ldx, *<mode>_ldx): Remove mode
	from (set ...).
	* config/rx/rx.md (movdi, movdf): Likewise.
	Likewise for define_peephole2s.

Comments

DJ Delorie May 8, 2015, 4:42 p.m. UTC | #1 
> ; This pattern is identical to the truncsipsi2 pattern except
> ; that it uses a SUBREG instead of a TRUNC.  It is needed in
> ; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
> ; into (SET:PSI (PSI)).
> 
> I'm not sure what that's supposed to mean (what's an SI set of a PSI
> subreg?), but I suspect removing the mode would lose information,
> so I left it alone.

MSP430 has 20-bit registers (PSImode-sized).  One register can hold an
HI or PSI sized value, but if you have an SI value it's stored as two
HI registers.

Thus, a PSImode value in a register is *not* just the 20 LSB of an
SImode value.  Also, a PSImode subset of an SI value is stored
different than a PSImode value on its own.

Thus, consider code like this:

(set (reg:SI 1)
     (subreg:PSI (reg:SI 2)))

(set (reg:PSI 1)
     (reg:PSI 2))

On most architectures, you'd say "these do the same thing" but on
MSP430 they don't.
Richard Sandiford May 9, 2015, 9:52 a.m. UTC | #2 
DJ Delorie <dj@redhat.com> writes:
>> ; This pattern is identical to the truncsipsi2 pattern except
>> ; that it uses a SUBREG instead of a TRUNC.  It is needed in
>> ; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
>> ; into (SET:PSI (PSI)).
>> 
>> I'm not sure what that's supposed to mean (what's an SI set of a PSI
>> subreg?), but I suspect removing the mode would lose information,
>> so I left it alone.
>
> MSP430 has 20-bit registers (PSImode-sized).  One register can hold an
> HI or PSI sized value, but if you have an SI value it's stored as two
> HI registers.
>
> Thus, a PSImode value in a register is *not* just the 20 LSB of an
> SImode value.  Also, a PSImode subset of an SI value is stored
> different than a PSImode value on its own.
>
> Thus, consider code like this:
>
> (set (reg:SI 1)
>      (subreg:PSI (reg:SI 2)))
>
> (set (reg:PSI 1)
>      (reg:PSI 2))
>
> On most architectures, you'd say "these do the same thing" but on
> MSP430 they don't.

What I was confused about is that the first set isn't valid rtl.
The SET_SRC and SET_DEST always have to have the same mode
(or VOIDmode in the case of a CONST_INT, etc., where the mode
is implicitly the same as the SET_DEST).  So wouldn't it have
to be:

  (set (reg:SI 1)
       (subreg:SI (reg:PSI 2)))

or:

  (set (reg:PSI 1)
       (subreg:PSI (reg:SI 2)))

?

Thanks,
Richard
DJ Delorie May 11, 2015, 4:55 p.m. UTC | #3 
> What I was confused about is that the first set isn't valid rtl.
> The SET_SRC and SET_DEST always have to have the same mode
> (or VOIDmode in the case of a CONST_INT, etc., where the mode
> is implicitly the same as the SET_DEST).  So wouldn't it have
> to be:
> 
>   (set (reg:SI 1)
>        (subreg:SI (reg:PSI 2)))
> 
> or:
> 
>   (set (reg:PSI 1)
>        (subreg:PSI (reg:SI 2)))
> 
> ?

If my memory doesn't match reality, I blame my memory.

I'd have to experiment a while to dig out the details, but the key
point is that a PSI in a reg is not stored the same as a PSI subreg of
an SI reg.  You have to keep that information across subregs or you
lose track of where the actual bits are.
diff mbox

Patch

Index: gcc/config/i386/i386.md
===================================================================
--- gcc/config/i386/i386.md	2015-05-08 14:42:57.823310127 +0100
+++ gcc/config/i386/i386.md	2015-05-08 14:43:12.515140307 +0100
@@ -18879,13 +18879,13 @@  (define_insn "*<mode>_<bndcheck>"
   [(set_attr "type" "mpxchk")])
 
 (define_expand "<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand")
-                       (unspec:BND
-		         [(mem:<bnd_ptr>
-			   (match_par_dup 3
-			     [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand")
-	                      (match_operand:<bnd_ptr> 2 "register_operand")]))]
-			 UNSPEC_BNDLDX))
+  [(parallel [(set (match_operand:BND 0 "register_operand")
+                   (unspec:BND
+		     [(mem:<bnd_ptr>
+		       (match_par_dup 3
+			 [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand")
+			  (match_operand:<bnd_ptr> 2 "register_operand")]))]
+		     UNSPEC_BNDLDX))
               (use (mem:BLK (match_dup 1)))])]
   "TARGET_MPX"
 {
@@ -18909,14 +18909,14 @@  (define_expand "<mode>_ldx"
 })
 
 (define_insn "*<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand" "=w")
-                       (unspec:BND
-		         [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
-			   [(unspec:<bnd_ptr>
-			     [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand" "Ti")
-	                      (match_operand:<bnd_ptr> 2 "register_operand" "l")]
-			    UNSPEC_BNDLDX_ADDR)])]
-			 UNSPEC_BNDLDX))
+  [(parallel [(set (match_operand:BND 0 "register_operand" "=w")
+		   (unspec:BND
+		     [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
+		       [(unspec:<bnd_ptr>
+			 [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand" "Ti")
+			  (match_operand:<bnd_ptr> 2 "register_operand" "l")]
+			UNSPEC_BNDLDX_ADDR)])]
+		     UNSPEC_BNDLDX))
               (use (mem:BLK (match_dup 1)))])]
   "TARGET_MPX"
   "bndldx\t{%3, %0|%0, %3}"
Index: gcc/config/rx/rx.md
===================================================================
--- gcc/config/rx/rx.md	2015-05-08 14:42:57.823310127 +0100
+++ gcc/config/rx/rx.md	2015-05-08 14:43:12.515140307 +0100
@@ -1734,9 +1734,9 @@  (define_peephole2
 					 (match_dup 2)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_commutative:SI (match_dup 2)
-					    (extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_commutative:SI (match_dup 2)
+					 (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1748,9 +1748,9 @@  (define_peephole2
 					 (match_dup 0)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_commutative:SI (match_dup 2)
-					    (extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_commutative:SI (match_dup 2)
+					 (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1762,9 +1762,9 @@  (define_peephole2
 				     (match_dup 0)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_noncomm:SI (match_dup 2)
-					(extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_noncomm:SI (match_dup 2)
+				     (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1775,9 +1775,9 @@  (define_peephole2
 	(memex_nocc:SI (match_dup 0)
 		       (match_dup 2)))]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(set:SI (match_dup 2)
-	   (memex_nocc:SI (match_dup 2)
-			  (extend_types:SI (match_dup 1))))]
+  [(set (match_dup 2)
+	(memex_nocc:SI (match_dup 2)
+		       (extend_types:SI (match_dup 1))))]
 )
 
 (define_peephole2
@@ -1787,9 +1787,9 @@  (define_peephole2
 	(memex_nocc:SI (match_dup 2)
 		       (match_dup 0)))]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(set:SI (match_dup 2)
-	   (memex_nocc:SI (match_dup 2)
-			  (extend_types:SI (match_dup 1))))]
+  [(set (match_dup 2)
+	(memex_nocc:SI (match_dup 2)
+		       (extend_types:SI (match_dup 1))))]
 )
 
 (define_insn "<memex_commutative:code>si3_<extend_types:code><small_int_modes:mode>"
@@ -2623,8 +2623,8 @@  (define_expand "pid_addr"
 )
 
 (define_insn "movdi"
-  [(set:DI (match_operand:DI 0 "nonimmediate_operand" "=rm")
-	   (match_operand:DI 1 "general_operand"      "rmi"))]
+  [(set (match_operand:DI 0 "nonimmediate_operand" "=rm")
+        (match_operand:DI 1 "general_operand"      "rmi"))]
   "TARGET_ENABLE_LRA"
   { return rx_gen_move_template (operands, false); }
   [(set_attr "length" "16")
@@ -2632,8 +2632,8 @@  (define_insn "movdi"
 )
 
 (define_insn "movdf"
-  [(set:DF (match_operand:DF 0 "nonimmediate_operand" "=rm")
-	   (match_operand:DF 1 "general_operand"      "rmi"))]
+  [(set (match_operand:DF 0 "nonimmediate_operand" "=rm")
+        (match_operand:DF 1 "general_operand"      "rmi"))]
   "TARGET_ENABLE_LRA"
   { return rx_gen_move_template (operands, false); }
   [(set_attr "length" "16")