diff mbox

Disallow side-effects in inline asm operands unless < or > constraint is used (PR middle-end/44492)

Message ID 20100620152558.GL7811@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 20, 2010, 3:25 p.m. UTC 
On Sun, Jun 20, 2010 at 11:16:15AM -0400, Hans-Peter Nilsson wrote:
> On Sun, 20 Jun 2010, Jakub Jelinek wrote:
> > Anyway, I guess I can live with not adding _ and saying that side-effects in
> > inline-asm memory is not allowed unless the constraint contains < or >.
> > Then PRE_MODIFY resp. POST_MODIFY would be allowed in
> > "m<>" or even just "m<" or "m>", assuming PRE_MODIFY resp. POST_MODIFY
> > is allowed in memory_address.
> >
> > The implementation would be roughly the same as the patch I've posted, just
> > </> would set incdec_ok instead of _.
> 
> And a much shorter documentation patch! :)
> (I guess mentioning that asm operands will not have side-effects
> unless < or > are explicitly mentioned would be right.)
> 
> > Advantage of doing it that way is backwards compatibility, "m<>" can be used
> > even with older gccs.
> >
> > If you prefer to do it that way, I can write a patch.
> 
> IMHO that's preferable, unless there's a known specific need for
> what "_m" brings in addition to "m<>" as suggested above.

Here is a new patch.  Ok for trunk?

2010-06-20  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/44492
	* recog.h (struct recog_data): Add is_asm field.
	* recog.c (asm_operand_ok, constrain_operands): If neither < nor > is
	present in constraints of inline-asm operand and memory operand
	contains {PRE,POST}_{INC,DEC,MODIFY}, return 0.
	(extract_insn): Initialize recog_data.is_asm.
	* doc/md.texi (Constraints): Document operand side-effect rules.

	* g++.dg/torture/pr44492.C: New test.



	Jakub

Comments

Hans-Peter Nilsson June 20, 2010, 4:43 p.m. UTC | #1 
On Sun, 20 Jun 2010, Jakub Jelinek wrote:
> Here is a new patch.  Ok for trunk?

No further comments from me, except "thanks for doing this".
(Not an approver.)

brgds, H-P
Jeff Law June 24, 2010, 5:29 p.m. UTC | #2 
On 06/20/10 09:25, Jakub Jelinek wrote:
> On Sun, Jun 20, 2010 at 11:16:15AM -0400, Hans-Peter Nilsson wrote:
>    
>> On Sun, 20 Jun 2010, Jakub Jelinek wrote:
>>      
>>> Anyway, I guess I can live with not adding _ and saying that side-effects in
>>> inline-asm memory is not allowed unless the constraint contains<  or>.
>>> Then PRE_MODIFY resp. POST_MODIFY would be allowed in
>>> "m<>" or even just "m<" or "m>", assuming PRE_MODIFY resp. POST_MODIFY
>>> is allowed in memory_address.
>>>
>>> The implementation would be roughly the same as the patch I've posted, just
>>> </>  would set incdec_ok instead of _.
>>>        
>> And a much shorter documentation patch! :)
>> (I guess mentioning that asm operands will not have side-effects
>> unless<  or>  are explicitly mentioned would be right.)
>>
>>      
>>> Advantage of doing it that way is backwards compatibility, "m<>" can be used
>>> even with older gccs.
>>>
>>> If you prefer to do it that way, I can write a patch.
>>>        
>> IMHO that's preferable, unless there's a known specific need for
>> what "_m" brings in addition to "m<>" as suggested above.
>>      
> Here is a new patch.  Ok for trunk?
>
> 2010-06-20  Jakub Jelinek<jakub@redhat.com>
>
> 	PR middle-end/44492
> 	* recog.h (struct recog_data): Add is_asm field.
> 	* recog.c (asm_operand_ok, constrain_operands): If neither<  nor>  is
> 	present in constraints of inline-asm operand and memory operand
> 	contains {PRE,POST}_{INC,DEC,MODIFY}, return 0.
> 	(extract_insn): Initialize recog_data.is_asm.
> 	* doc/md.texi (Constraints): Document operand side-effect rules.
>
> 	* g++.dg/torture/pr44492.C: New test.
>    
OK.

jeff
diff mbox

Patch

--- gcc/recog.h.jj	2010-06-14 17:14:16.481241913 +0200
+++ gcc/recog.h	2010-06-20 16:15:01.218441495 +0200
@@ -230,6 +230,9 @@  struct recog_data
   /* The number of alternatives in the constraints for the insn.  */
   char n_alternatives;
 
+  /* True if insn is ASM_OPERANDS.  */
+  bool is_asm;
+
   /* Specifies whether an insn alternative is enabled using the
      `enabled' attribute in the insn pattern definition.  For back
      ends not using the `enabled' attribute the array fields are
--- gcc/recog.c.jj	2010-06-14 17:14:16.466197350 +0200
+++ gcc/recog.c	2010-06-20 16:17:10.676210868 +0200
@@ -1601,6 +1601,9 @@  int
 asm_operand_ok (rtx op, const char *constraint, const char **constraints)
 {
   int result = 0;
+#ifdef AUTO_INC_DEC
+  bool incdec_ok = false;
+#endif
 
   /* Use constrain_operands after reload.  */
   gcc_assert (!reload_completed);
@@ -1608,7 +1611,7 @@  asm_operand_ok (rtx op, const char *cons
   /* Empty constraint string is the same as "X,...,X", i.e. X for as
      many alternatives as required to match the other operands.  */
   if (*constraint == '\0')
-    return 1;
+    result = 1;
 
   while (*constraint)
     {
@@ -1685,6 +1688,9 @@  asm_operand_ok (rtx op, const char *cons
 		  || GET_CODE (XEXP (op, 0)) == PRE_DEC
 		  || GET_CODE (XEXP (op, 0)) == POST_DEC))
 	    result = 1;
+#ifdef AUTO_INC_DEC
+	  incdec_ok = true;
+#endif
 	  break;
 
 	case '>':
@@ -1693,6 +1699,9 @@  asm_operand_ok (rtx op, const char *cons
 		  || GET_CODE (XEXP (op, 0)) == PRE_INC
 		  || GET_CODE (XEXP (op, 0)) == POST_INC))
 	    result = 1;
+#ifdef AUTO_INC_DEC
+	  incdec_ok = true;
+#endif
 	  break;
 
 	case 'E':
@@ -1814,6 +1823,23 @@  asm_operand_ok (rtx op, const char *cons
 	return 0;
     }
 
+#ifdef AUTO_INC_DEC
+  /* For operands without < or > constraints reject side-effects.  */
+  if (!incdec_ok && result && MEM_P (op))
+    switch (GET_CODE (XEXP (op, 0)))
+      {
+      case PRE_INC:
+      case POST_INC:
+      case PRE_DEC:
+      case POST_DEC:
+      case PRE_MODIFY:
+      case POST_MODIFY:
+	return 0;
+      default:
+	break;
+      }
+#endif
+
   return result;
 }
 
@@ -2039,6 +2065,7 @@  extract_insn (rtx insn)
   recog_data.n_operands = 0;
   recog_data.n_alternatives = 0;
   recog_data.n_dups = 0;
+  recog_data.is_asm = false;
 
   switch (GET_CODE (body))
     {
@@ -2085,6 +2112,7 @@  extract_insn (rtx insn)
 	      while (*p)
 		recog_data.n_alternatives += (*p++ == ',');
 	    }
+	  recog_data.is_asm = true;
 	  break;
 	}
       fatal_insn_not_found (insn);
@@ -2699,6 +2727,30 @@  constrain_operands (int strict)
 		    = recog_data.operand[funny_match[funny_match_index].this_op];
 		}
 
+#ifdef AUTO_INC_DEC
+	      /* For operands without < or > constraints reject side-effects.  */
+	      if (recog_data.is_asm)
+		{
+		  for (opno = 0; opno < recog_data.n_operands; opno++)
+		    if (MEM_P (recog_data.operand[opno]))
+		      switch (GET_CODE (XEXP (recog_data.operand[opno], 0)))
+			{
+			case PRE_INC:
+			case POST_INC:
+			case PRE_DEC:
+			case POST_DEC:
+			case PRE_MODIFY:
+			case POST_MODIFY:
+			  if (strchr (recog_data.constraints[opno], '<') == NULL
+			      || strchr (recog_data.constraints[opno], '>')
+				 == NULL)
+			    return 0;
+			  break;
+			default:
+			  break;
+			}
+		}
+#endif
 	      return 1;
 	    }
 	}
--- gcc/doc/md.texi.jj	2010-06-14 17:14:16.000000000 +0200
+++ gcc/doc/md.texi	2010-06-20 16:28:51.199197446 +0200
@@ -1052,6 +1052,10 @@  an operand may be in a register, and whi
 operand can be a memory reference, and which kinds of address; whether the
 operand may be an immediate constant, and which possible values it may
 have.  Constraints can also require two operands to match.
+Side-effects aren't allowed in operands of inline @code{asm}, unless
+@samp{<} or @samp{>} constraints are used, because there is no guarantee
+that the side-effects will happen exactly once in an instruction that can update
+the addressing register.
 
 @ifset INTERNALS
 @menu
@@ -1129,12 +1133,21 @@  would fit the @samp{m} constraint but no
 @cindex @samp{<} in constraint
 @item @samp{<}
 A memory operand with autodecrement addressing (either predecrement or
-postdecrement) is allowed.
+postdecrement) is allowed.  In inline @code{asm} this constraint is only
+allowed if the operand is used exactly once in an instruction that can
+handle the side-effects.  Not using an operand with @samp{<} in constraint
+string in the inline @code{asm} pattern at all or using it in multiple
+instructions isn't valid, because the side-effects wouldn't be performed
+or would be performed more than once.  Furthermore, on some targets
+the operand with @samp{<} in constraint string must be accompanied by
+special instruction suffixes like @code{%U0} instruction suffix on PowerPC
+or @code{%P0} on IA-64.
 
 @cindex @samp{>} in constraint
 @item @samp{>}
 A memory operand with autoincrement addressing (either preincrement or
-postincrement) is allowed.
+postincrement) is allowed.  In inline @code{asm} the same restrictions
+as for @samp{<} apply.
 
 @cindex @samp{r} in constraint
 @cindex registers in constraints
--- gcc/testsuite/g++.dg/torture/pr44492.C.jj	2010-06-20 16:15:01.221230004 +0200
+++ gcc/testsuite/g++.dg/torture/pr44492.C	2010-06-20 16:15:01.221230004 +0200
@@ -0,0 +1,31 @@ 
+// PR middle-end/44492
+// { dg-do run }
+
+struct T { unsigned long p; };
+struct S { T a, b, c; unsigned d; };
+
+__attribute__((noinline))
+void
+bar (const T &x, const T &y)
+{
+  if (x.p != 0x2348 || y.p != 0x2346)
+    __builtin_abort ();
+}
+
+__attribute__((noinline))
+void
+foo (S &s, T e)
+{
+  unsigned long a = e.p;
+  unsigned long b = s.b.p;
+  __asm__ volatile ("" : : "rm" (a), "rm" (b));
+  bar (e, s.b);
+}
+
+int
+main ()
+{
+  S s = { { 0x2345 }, { 0x2346 }, { 0x2347 }, 6 };
+  T t = { 0x2348 };
+  foo (s, t);
+}