diff mbox

Require '%' to be at the beginning of a constraint string

Message ID 8738ftbs3o.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford May 28, 2014, 7:50 p.m. UTC 
Thanks for the review.

Jeff Law <law@redhat.com> writes:
> On 05/26/14 13:21, Richard Sandiford wrote:
>>> If we're going to change it, then clearly the docs need to change and
>>> ideally we'd statically check the port's constraints during the build
>>> process to ensure they meet the tighter definition.
>>
>> OK, how does this look?  I built a cc1 for one target per config/
>> directory to (try to) check that there were no remaining cases.
>>
>> This means that we will silently ignore '%'s that are embedded in the
>> middle of an asm constraint string, but in a way that's already true for
>> most places that check for commutativity.  An error seems a bit extreme
>> when '%' is only a hint.  If we want a warning, what should the option
>> be called?  And should it be under -Wall, -Wextra, or on by default?
>>
>> Tested on x86_64-linux-gnu.  OK to install?
> OK.  My initial thought on adding a warning was to weed out bad 
> constraints.  You've already done that for the in-tree ports.  I'm a lot 
> less inclined to do much more here to help the out-of-tree ports, so 
> upon further review, let's not worry about the warning unless you've 
> already got it ready to go :-)

Well, the new genoutput.c error should catch problems in out-of-tree ports.
It's just asms where the non-initial '%'s would be silently accepted
and have no effect.

David W suggested off-list that I add "Only input operands can use
@samp{%}." to the documention as well.  That seemed like it was
obviously an improvement so I applied the patch with that change (below).

Thanks,
Richard


gcc/
	* doc/md.texi: Document that the % constraint character must
	be at the beginning of the string.
	* genoutput.c (validate_insn_alternatives): Check that '=',
	'+' and '%' only appear at the beginning of a constraint.
	* ira.c (commutative_constraint_p): Delete.
	(ira_get_dup_out_num): Expect the '%' commutativity marker to be
	at the start of the string.
	* config/alpha/alpha.md (*movmemdi_1, *clrmemdi_1): Remove
	duplicate '='s.
	* config/arm/neon.md (bicdi3_neon): Likewise.
	* config/iq2000/iq2000.md (addsi3_internal, subsi3_internal, sgt_si)
	(slt_si, sltu_si): Likewise.
	* config/vax/vax.md (sbcdi3): Likewise.
	* config/h8300/h8300.md (*cmpstz): Remove duplicate '+'.
	* config/arc/arc.md (mulsi_600, mulsidi_600, umulsidi_600)
	(mul64): Move '%' to beginning of constraint.
	* config/arm/arm.md (*xordi3_insn): Likewise.
	* config/nds32/nds32.md (add<mode>3, mulsi3, andsi3, iorsi3)
	(xorsi3): Likewise.

Comments

Jeff Law May 30, 2014, 4:24 p.m. UTC | #1 
On 05/28/14 13:50, Richard Sandiford wrote:
> Thanks for the review.
>
> Jeff Law <law@redhat.com> writes:
>> On 05/26/14 13:21, Richard Sandiford wrote:
>>>> If we're going to change it, then clearly the docs need to change and
>>>> ideally we'd statically check the port's constraints during the build
>>>> process to ensure they meet the tighter definition.
>>>
>>> OK, how does this look?  I built a cc1 for one target per config/
>>> directory to (try to) check that there were no remaining cases.
>>>
>>> This means that we will silently ignore '%'s that are embedded in the
>>> middle of an asm constraint string, but in a way that's already true for
>>> most places that check for commutativity.  An error seems a bit extreme
>>> when '%' is only a hint.  If we want a warning, what should the option
>>> be called?  And should it be under -Wall, -Wextra, or on by default?
>>>
>>> Tested on x86_64-linux-gnu.  OK to install?
>> OK.  My initial thought on adding a warning was to weed out bad
>> constraints.  You've already done that for the in-tree ports.  I'm a lot
>> less inclined to do much more here to help the out-of-tree ports, so
>> upon further review, let's not worry about the warning unless you've
>> already got it ready to go :-)
>
> Well, the new genoutput.c error should catch problems in out-of-tree ports.
Right.

> It's just asms where the non-initial '%'s would be silently accepted
> and have no effect.
Right.  And I believe that it's conservatively correct -- so we're OK 
here as well.

>
> David W suggested off-list that I add "Only input operands can use
> @samp{%}." to the documention as well.  That seemed like it was
> obviously an improvement so I applied the patch with that change (below).
Funny I was thinking about that when looking at the arc changes.  I saw 
the '%' on operand0 and was confused thinking it made no sense to have a 
'%' for an output operand.  Then I realized that 0/1 were inputs and 2 
was the output.

Thanks for pushing this through.

jeff
diff mbox

Patch

Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi	2014-05-27 11:23:48.676099753 +0100
+++ gcc/doc/md.texi	2014-05-27 11:23:53.289138410 +0100
@@ -1589,7 +1589,10 @@  See, for example, the @samp{mulsi3} insn
 Declares the instruction to be commutative for this operand and the
 following operand.  This means that the compiler may interchange the
 two operands if that is the cheapest way to make all operands fit the
-constraints.
+constraints.  @samp{%} applies to all alternatives and must appear as
+the first character in the constraint.  Only input operands can use
+@samp{%}.
+
 @ifset INTERNALS
 This is often used in patterns for addition instructions
 that really have only two operands: the result must go in one of the
Index: gcc/genoutput.c
===================================================================
--- gcc/genoutput.c	2014-05-27 11:23:48.661099627 +0100
+++ gcc/genoutput.c	2014-05-27 11:23:53.284138368 +0100
@@ -781,6 +781,11 @@  validate_insn_alternatives (struct data
 
 	for (p = d->operand[start].constraint; (c = *p); p += len)
 	  {
+	    if ((c == '%' || c == '=' || c == '+')
+		&& p != d->operand[start].constraint)
+	      error_with_line (d->lineno,
+			       "character '%c' can only be used at the"
+			       " beginning of a constraint string", c);
 #ifdef USE_MD_CONSTRAINTS
 	    if (ISSPACE (c) || strchr (indep_constraints, c))
 	      len = 1;
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	2014-05-27 11:23:48.679099778 +0100
+++ gcc/ira.c	2014-05-27 11:24:23.746394430 +0100
@@ -1770,34 +1770,6 @@  setup_prohibited_mode_move_regs (void)
 
 
 
-/* Return TRUE if the operand constraint STR is commutative.  */
-static bool
-commutative_constraint_p (const char *str)
-{
-  int c;
-
-  alternative_mask enabled = recog_data.enabled_alternatives;
-  for (;;)
-    {
-      c = *str;
-      if (c == '\0')
-	break;
-      str += CONSTRAINT_LEN (c, str);
-      if (c == '#')
-	enabled &= ~ALTERNATIVE_BIT (0);
-      else if (c == ',')
-	enabled >>= 1;
-      else if (enabled & 1)
-	{
-	  /* Usually `%' is the first constraint character but the
-	     documentation does not require this.  */
-	  if (c == '%')
-	    return true;
-	}
-    }
-  return false;
-}
-
 /* Setup possible alternatives in ALTS for INSN.  */
 void
 ira_setup_alts (rtx insn, HARD_REG_SET &alts)
@@ -2099,10 +2071,9 @@  ira_get_dup_out_num (int op_num, HARD_RE
       if (use_commut_op_p)
 	break;
       use_commut_op_p = true;
-      if (commutative_constraint_p (recog_data.constraints[op_num]))
+      if (recog_data.constraints[op_num][0] == '%')
 	str = recog_data.constraints[op_num + 1];
-      else if (op_num > 0 && commutative_constraint_p (recog_data.constraints
-						       [op_num - 1]))
+      else if (op_num > 0 && recog_data.constraints[op_num - 1][0] == '%')
 	str = recog_data.constraints[op_num - 1];
       else
 	break;
Index: gcc/config/alpha/alpha.md
===================================================================
--- gcc/config/alpha/alpha.md	2014-05-27 11:23:48.663099644 +0100
+++ gcc/config/alpha/alpha.md	2014-05-27 11:23:53.285138376 +0100
@@ -4764,7 +4764,7 @@  (define_expand "movmemdi"
   "operands[4] = gen_rtx_SYMBOL_REF (Pmode, \"OTS$MOVE\");")
 
 (define_insn "*movmemdi_1"
-  [(set (match_operand:BLK 0 "memory_operand" "=m,=m")
+  [(set (match_operand:BLK 0 "memory_operand" "=m,m")
 	(match_operand:BLK 1 "memory_operand" "m,m"))
    (use (match_operand:DI 2 "nonmemory_operand" "r,i"))
    (use (match_operand:DI 3 "immediate_operand"))
@@ -4831,7 +4831,7 @@  (define_expand "setmemdi"
 })
 
 (define_insn "*clrmemdi_1"
-  [(set (match_operand:BLK 0 "memory_operand" "=m,=m")
+  [(set (match_operand:BLK 0 "memory_operand" "=m,m")
 		   (const_int 0))
    (use (match_operand:DI 1 "nonmemory_operand" "r,i"))
    (use (match_operand:DI 2 "immediate_operand"))
Index: gcc/config/arm/neon.md
===================================================================
--- gcc/config/arm/neon.md	2014-05-27 11:23:48.665099661 +0100
+++ gcc/config/arm/neon.md	2014-05-27 11:23:53.286138385 +0100
@@ -728,7 +728,7 @@  (define_insn "bic<mode>3_neon"
 
 ;; Compare to *anddi_notdi_di.
 (define_insn "bicdi3_neon"
-  [(set (match_operand:DI 0 "s_register_operand" "=w,?=&r,?&r")
+  [(set (match_operand:DI 0 "s_register_operand" "=w,?&r,?&r")
         (and:DI (not:DI (match_operand:DI 2 "s_register_operand" "w,r,0"))
 		(match_operand:DI 1 "s_register_operand" "w,0,r")))]
   "TARGET_NEON"
Index: gcc/config/iq2000/iq2000.md
===================================================================
--- gcc/config/iq2000/iq2000.md	2014-05-27 11:23:48.681099795 +0100
+++ gcc/config/iq2000/iq2000.md	2014-05-27 11:23:53.292138435 +0100
@@ -260,7 +260,7 @@  (define_expand "addsi3"
   "")
 
 (define_insn "addsi3_internal"
-  [(set (match_operand:SI 0 "register_operand" "=d,=d")
+  [(set (match_operand:SI 0 "register_operand" "=d,d")
 	(plus:SI (match_operand:SI 1 "reg_or_0_operand" "dJ,dJ")
 		 (match_operand:SI 2 "arith_operand" "d,I")))]
   ""
@@ -286,7 +286,7 @@  (define_expand "subsi3"
   "")
 
 (define_insn "subsi3_internal"
-  [(set (match_operand:SI 0 "register_operand" "=d,=d")
+  [(set (match_operand:SI 0 "register_operand" "=d,d")
 	(minus:SI (match_operand:SI 1 "reg_or_0_operand" "dJ,dJ")
 		  (match_operand:SI 2 "arith_operand" "d,I")))]
   ""
@@ -1229,7 +1229,7 @@  (define_insn "sne_si_zero"
    (set_attr "mode"	"SI")])
 
 (define_insn "sgt_si"
-  [(set (match_operand:SI 0 "register_operand" "=d,=d")
+  [(set (match_operand:SI 0 "register_operand" "=d,d")
 	(gt:SI (match_operand:SI 1 "register_operand" "d,d")
 	       (match_operand:SI 2 "reg_or_0_operand" "d,J")))]
   ""
@@ -1240,7 +1240,7 @@  (define_insn "sgt_si"
    (set_attr "mode"	"SI,SI")])
 
 (define_insn "slt_si"
-  [(set (match_operand:SI 0 "register_operand" "=d,=d")
+  [(set (match_operand:SI 0 "register_operand" "=d,d")
 	(lt:SI (match_operand:SI 1 "register_operand" "d,d")
 	       (match_operand:SI 2 "arith_operand" "d,I")))]
   ""
@@ -1273,7 +1273,7 @@  (define_insn "sgtu_si"
    (set_attr "mode"	"SI")])
 
 (define_insn "sltu_si"
-  [(set (match_operand:SI 0 "register_operand" "=d,=d")
+  [(set (match_operand:SI 0 "register_operand" "=d,d")
 	(ltu:SI (match_operand:SI 1 "register_operand" "d,d")
 		(match_operand:SI 2 "arith_operand" "d,I")))]
   ""
Index: gcc/config/vax/vax.md
===================================================================
--- gcc/config/vax/vax.md	2014-05-27 11:23:48.666099669 +0100
+++ gcc/config/vax/vax.md	2014-05-27 11:23:53.286138385 +0100
@@ -423,7 +423,7 @@  (define_expand "subdi3"
   "vax_expand_addsub_di_operands (operands, MINUS); DONE;")
 
 (define_insn "sbcdi3"
-  [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,=Rr")
+  [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,Rr")
 	(minus:DI (match_operand:DI 1 "general_addsub_di_operand" "0,I")
 		  (match_operand:DI 2 "general_addsub_di_operand" "nRr,Rr")))]
   "TARGET_QMATH"
Index: gcc/config/h8300/h8300.md
===================================================================
--- gcc/config/h8300/h8300.md	2014-05-27 11:23:48.668099686 +0100
+++ gcc/config/h8300/h8300.md	2014-05-27 11:23:53.288138401 +0100
@@ -3589,7 +3589,7 @@  (define_insn "*bstzhireg"
   [(set_attr "cc" "clobber")])
 
 (define_insn_and_split "*cmpstz"
-  [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,+WU")
+  [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,WU")
 			 (const_int 1)
 			 (match_operand:QI 1 "immediate_operand" "n,n"))
 	(match_operator:QI 2 "eqne_operator"
Index: gcc/config/arc/arc.md
===================================================================
--- gcc/config/arc/arc.md	2014-05-27 11:23:48.650099535 +0100
+++ gcc/config/arc/arc.md	2014-05-27 11:23:53.280138334 +0100
@@ -1698,7 +1698,7 @@  (define_insn "mac_600"
 
 (define_insn "mulsi_600"
   [(set (match_operand:SI 2 "mlo_operand" "")
-	(mult:SI (match_operand:SI 0 "register_operand"  "Rcq#q,c,c,%c")
+	(mult:SI (match_operand:SI 0 "register_operand"  "%Rcq#q,c,c,c")
 		 (match_operand:SI 1 "nonmemory_operand" "Rcq#q,cL,I,Cal")))
    (clobber (match_operand:SI 3 "mhi_operand" ""))]
   "TARGET_MUL64_SET"
@@ -1750,7 +1750,7 @@  (define_insn "mulsi3_600_lib"
 (define_insn "mulsidi_600"
   [(set (reg:DI MUL64_OUT_REG)
 	(mult:DI (sign_extend:DI
-		   (match_operand:SI 0 "register_operand"  "Rcq#q,c,c,%c"))
+		   (match_operand:SI 0 "register_operand"  "%Rcq#q,c,c,c"))
 		 (sign_extend:DI
 ; assembler issue for "I", see mulsi_600
 ;		   (match_operand:SI 1 "register_operand" "Rcq#q,cL,I,Cal"))))]
@@ -1766,7 +1766,7 @@  (define_insn "mulsidi_600"
 (define_insn "umulsidi_600"
   [(set (reg:DI MUL64_OUT_REG)
 	(mult:DI (zero_extend:DI
-		   (match_operand:SI 0 "register_operand"  "c,c,%c"))
+		   (match_operand:SI 0 "register_operand"  "%c,c,c"))
 		 (sign_extend:DI
 ; assembler issue for "I", see mulsi_600
 ;		   (match_operand:SI 1 "register_operand" "cL,I,Cal"))))]
@@ -4134,7 +4134,7 @@  (define_insn "swap"
 
 ;; FIXME: an intrinsic for multiply is daft.  Can we remove this?
 (define_insn "mul64"
-  [(unspec [(match_operand:SI 0 "general_operand" "q,r,r,%r")
+  [(unspec [(match_operand:SI 0 "general_operand" "%q,r,r,r")
 		     (match_operand:SI 1 "general_operand" "q,rL,I,Cal")]
 		   UNSPEC_MUL64)]
   "TARGET_MUL64_SET"
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	2014-05-27 11:23:48.654099569 +0100
+++ gcc/config/arm/arm.md	2014-05-27 11:23:53.282138351 +0100
@@ -3169,7 +3169,7 @@  (define_expand "xordi3"
 
 (define_insn_and_split "*xordi3_insn"
   [(set (match_operand:DI         0 "s_register_operand" "=w,&r,&r,&r,&r,?w")
-	(xor:DI (match_operand:DI 1 "s_register_operand" "w ,%0,r ,0 ,r ,w")
+	(xor:DI (match_operand:DI 1 "s_register_operand" "%w ,0,r ,0 ,r ,w")
 		(match_operand:DI 2 "arm_xordi_operand"  "w ,r ,r ,Dg,Dg,w")))]
   "TARGET_32BIT && !TARGET_IWMMXT"
 {
Index: gcc/config/nds32/nds32.md
===================================================================
--- gcc/config/nds32/nds32.md	2014-05-27 11:23:48.656099585 +0100
+++ gcc/config/nds32/nds32.md	2014-05-27 11:23:53.283138359 +0100
@@ -261,7 +261,7 @@  (define_insn "extend<mode>si2"
 
 (define_insn "add<mode>3"
   [(set (match_operand:QIHISI 0 "register_operand"                   "=   d,    l,    d,    l,  d, l,    k,    l,    r, r")
-	(plus:QIHISI (match_operand:QIHISI 1 "register_operand"      "    0,    l,    0,    l, %0, l,    0,    k,    r, r")
+	(plus:QIHISI (match_operand:QIHISI 1 "register_operand"      "%   0,    l,    0,    l,  0, l,    0,    k,    r, r")
 		     (match_operand:QIHISI 2 "nds32_rimm15s_operand" " In05, In03, Iu05, Iu03,  r, l, Is10, Iu06, Is15, r")))]
   ""
 {
@@ -382,9 +382,9 @@  (define_insn "*sub_srli"
 ;; Multiplication instructions.
 
 (define_insn "mulsi3"
-  [(set (match_operand:SI 0 "register_operand"          "= w, r")
-	(mult:SI (match_operand:SI 1 "register_operand" " %0, r")
-		 (match_operand:SI 2 "register_operand" "  w, r")))]
+  [(set (match_operand:SI 0 "register_operand"          "=w, r")
+	(mult:SI (match_operand:SI 1 "register_operand" "%0, r")
+		 (match_operand:SI 2 "register_operand" " w, r")))]
   ""
   "@
   mul33\t%0, %2
@@ -489,9 +489,9 @@  (define_insn "bitc"
 )
 
 (define_insn "andsi3"
-  [(set (match_operand:SI 0 "register_operand"         "= w, r,    l,    l,    l,    l,    l,    l,    r,   r,     r,    r,    r")
-	(and:SI (match_operand:SI 1 "register_operand" " %0, r,    l,    l,    l,    l,    0,    0,    r,   r,     r,    r,    r")
-		(match_operand:SI 2 "general_operand"  "  w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))]
+  [(set (match_operand:SI 0 "register_operand"         "=w, r,    l,    l,    l,    l,    l,    l,    r,   r,     r,    r,    r")
+	(and:SI (match_operand:SI 1 "register_operand" "%0, r,    l,    l,    l,    l,    0,    0,    r,   r,     r,    r,    r")
+		(match_operand:SI 2 "general_operand"  " w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))]
   ""
 {
   HOST_WIDE_INT mask = INTVAL (operands[2]);
@@ -585,9 +585,9 @@  (define_insn "*and_srli"
 ;; For V3/V3M ISA, we have 'or33' instruction.
 ;; So we can identify 'or Rt3,Rt3,Ra3' case and set its length to be 2.
 (define_insn "iorsi3"
-  [(set (match_operand:SI 0 "register_operand"         "= w, r,    r,    r")
-	(ior:SI (match_operand:SI 1 "register_operand" " %0, r,    r,    r")
-		(match_operand:SI 2 "general_operand"  "  w, r, Iu15, Ie15")))]
+  [(set (match_operand:SI 0 "register_operand"         "=w, r,    r,    r")
+	(ior:SI (match_operand:SI 1 "register_operand" "%0, r,    r,    r")
+		(match_operand:SI 2 "general_operand"  " w, r, Iu15, Ie15")))]
   ""
 {
   int one_position;
@@ -645,9 +645,9 @@  (define_insn "*or_srli"
 ;; For V3/V3M ISA, we have 'xor33' instruction.
 ;; So we can identify 'xor Rt3,Rt3,Ra3' case and set its length to be 2.
 (define_insn "xorsi3"
-  [(set (match_operand:SI 0 "register_operand"         "= w, r,    r,    r")
-	(xor:SI (match_operand:SI 1 "register_operand" " %0, r,    r,    r")
-		(match_operand:SI 2 "general_operand"  "  w, r, Iu15, It15")))]
+  [(set (match_operand:SI 0 "register_operand"         "=w, r,    r,    r")
+	(xor:SI (match_operand:SI 1 "register_operand" "%0, r,    r,    r")
+		(match_operand:SI 2 "general_operand"  " w, r, Iu15, It15")))]
   ""
 {
   int one_position;