diff mbox

[v2] x86: allow to suppress default clobbers added to asm()s

Message ID 577D328702000078000FBACF@prv-mh.provo.novell.com
State New
Headers show

Commit Message

Jan Beulich July 6, 2016, 2:32 p.m. UTC 
While it always seemed wrong to me that there's no way to avoid the
default "flags" and "fpsr" clobbers, the regression the fix for
PR/60663 introduced (see PR/63637) makes it even more desirable to have
such a mechanism: This way, at least asm()s with a single output and no
explicit clobbers could again have been made subject to CSE even with
that bug unfixed.
---
There wasn't much feedback on v1
(https://gcc.gnu.org/ml/gcc-patches/2014-10/msg03251.html)
and the feedback I did get from Jeff I didn't really mean to address
in this version:

> I really don't like having an option that's globally applied for this
> feature. THough I am OK with having a mechanism to avoid
> implicit clobbers on specific ASMs.

I don't really understand what's wrong with a command line option
allowing to state this globally for a source file or even entire project.

> Why use negative numbers for the hard register numbers? I
> wouldn't be at all surprised if lots of random code assumes
> register numbers are always positive.

I'm lacking an idea (or suggestion) of a better alternative. Using
positive numbers resulted in far more problems, as such registers
then got accepted elsewhere as valid too.

> I don't like adding new registers with special names like !foo.
> Instead I think that listing "!cc" or something similar in the asm
> itself if it doesn't clobber the cc register would be better. 

I didn't really understand what was meant here, i.e. how the
proposed alternative was supposed to look like in an actual
asm().

gcc/
2016-07-06  Jan Beulich  <jbeulich@suse.com>

	* cfgexpand.c (expand_asm_stmt): Cope with negative register
	numbers.
	* config/i386/i386.c (ix86_target_string): Add
	-mno-default-asm-clobbers.
	(ix86_valid_target_attribute_inner_p): Handle
	-m{,no-}default-asm-clobbers.
	(ix86_md_asm_adjust): Handle "inverse" clobbers.
	* config/i386/i386.h (NOFLAGS_REGNUM, NOFPSR_REGNUM): Define.
	(ADDITIONAL_REGISTER_NAMES): Add "!flags" and "!fpsr".
	(OVERLAPPING_REGISTER_NAMES): Define.
	* config/i386/i386.opt: Add mdefault-asm-clobbers and
	mno-default-asm-clobbers.
	* varasm.c (decode_reg_name_and_count): Permit negative
	register numbers from ADDITIONAL_REGISTER_NAMES.

gcc/testsuite/
2016-07-06  Jan Beulich  <jbeulich@suse.com>

	* gcc.target/i386/20060218-1.c: Adjust expected error.
	* gcc.target/i386/invclbr[123].c: New.

Comments

Jeff Law July 20, 2016, 10:35 p.m. UTC | #1 
On 07/06/2016 08:32 AM, Jan Beulich wrote:
> While it always seemed wrong to me that there's no way to avoid the
> default "flags" and "fpsr" clobbers, the regression the fix for
> PR/60663 introduced (see PR/63637) makes it even more desirable to have
> such a mechanism: This way, at least asm()s with a single output and no
> explicit clobbers could again have been made subject to CSE even with
> that bug unfixed.
> ---
> There wasn't much feedback on v1
> (https://gcc.gnu.org/ml/gcc-patches/2014-10/msg03251.html)
> and the feedback I did get from Jeff I didn't really mean to address
> in this version:
>
>> I really don't like having an option that's globally applied for this
>> feature. THough I am OK with having a mechanism to avoid
>> implicit clobbers on specific ASMs.
>
> I don't really understand what's wrong with a command line option
> allowing to state this globally for a source file or even entire project.
It's been a while, but I believe my concern was that if you flip a flag 
globally, then it's an all-or-nothing proposition.  ie, you either have 
default clobbers or you do not have default clobbers for the affected 
TU.  While that may be OK for some projects, I think that folks will 
want/need this applied on a per-asm basis far far more often.  That's 
just my opinion -- I don't have anything hard to back that up.


>
>> Why use negative numbers for the hard register numbers? I
>> wouldn't be at all surprised if lots of random code assumes
>> register numbers are always positive.
>
> I'm lacking an idea (or suggestion) of a better alternative. Using
> positive numbers resulted in far more problems, as such registers
> then got accepted elsewhere as valid too.
Sadly, I don't have a better suggestion.  Perhaps Bernd or someone else 
has ideas on how to represent.

>
>> I don't like adding new registers with special names like !foo.
>> Instead I think that listing "!cc" or something similar in the asm
>> itself if it doesn't clobber the cc register would be better.
>
> I didn't really understand what was meant here, i.e. how the
> proposed alternative was supposed to look like in an actual
> asm().
I think what I was suggesting was to use the !<name> syntax, but not as 
a distinct register name.  ie, the ! essentially could be applied to any 
register and would be handled outside the target specific bits.

Though I think this implies a new API for decode_reg_name_and_count 
where certain negative values have special meaning.  But that ought to 
be trivial.


Jeff
diff mbox

Patch

--- 2016-06-30/gcc/cfgexpand.c
+++ 2016-06-30/gcc/cfgexpand.c
@@ -2884,26 +2884,18 @@  expand_asm_stmt (gasm *stmt)
 	  int nregs, j;
 
 	  j = decode_reg_name_and_count (regname, &nregs);
-	  if (j < 0)
+	  if (j == -2)
 	    {
-	      if (j == -2)
-		{
-		  /* ??? Diagnose during gimplification?  */
-		  error ("unknown register name %qs in %<asm%>", regname);
-		}
-	      else if (j == -4)
-		{
-		  rtx x = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode));
-		  clobber_rvec.safe_push (x);
-		}
-	      else
-		{
-		  /* Otherwise we should have -1 == empty string
-		     or -3 == cc, which is not a register.  */
-		  gcc_assert (j == -1 || j == -3);
-		}
+	      /* ??? Diagnose during gimplification?  */
+	      error ("unknown register name %qs in %<asm%>", regname);
 	    }
-	  else
+	  else if (j == -4)
+	    {
+	      rtx x = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode));
+	      clobber_rvec.safe_push (x);
+	    }
+	  else if (j != -1 /* empty string */
+		   && j != -3 /* cc, which is not a register */)
 	    for (int reg = j; reg < j + nregs; reg++)
 	      {
 		/* Clobbering the PIC register is an error.  */
@@ -2915,7 +2907,8 @@  expand_asm_stmt (gasm *stmt)
 		    return;
 		  }
 
-	        SET_HARD_REG_BIT (clobbered_regs, reg);
+		if (reg >= 0)
+		  SET_HARD_REG_BIT (clobbered_regs, reg);
 	        rtx x = gen_rtx_REG (reg_raw_mode[reg], reg);
 		clobber_rvec.safe_push (x);
 	      }
--- 2016-06-30/gcc/config/i386/i386.c
+++ 2016-06-30/gcc/config/i386/i386.c
@@ -4207,6 +4207,7 @@  ix86_target_string (HOST_WIDE_INT isa, i
     { "-minline-stringops-dynamically",	MASK_INLINE_STRINGOPS_DYNAMICALLY },
     { "-mms-bitfields",			MASK_MS_BITFIELD_LAYOUT },
     { "-mno-align-stringops",		MASK_NO_ALIGN_STRINGOPS },
+    { "-mno-default-asm-clobbers",	MASK_NO_DEFAULT_ASM_CLOBBERS },
     { "-mno-fancy-math-387",		MASK_NO_FANCY_MATH_387 },
     { "-mno-push-args",			MASK_NO_PUSH_ARGS },
     { "-mno-red-zone",			MASK_NO_RED_ZONE },
@@ -6488,6 +6489,10 @@  ix86_valid_target_attribute_inner_p (tre
 		  OPT_mno_align_stringops,
 		  MASK_NO_ALIGN_STRINGOPS),
 
+    IX86_ATTR_NO ("default-asm-clobbers",
+		  OPT_mno_default_asm_clobbers,
+		  MASK_NO_DEFAULT_ASM_CLOBBERS),
+
     IX86_ATTR_YES ("recip",
 		   OPT_mrecip,
 		   MASK_RECIP),
@@ -48384,8 +48389,38 @@  ix86_md_asm_adjust (vec<rtx> &outputs, v
 		    vec<const char *> &constraints,
 		    vec<rtx> &clobbers, HARD_REG_SET &clobbered_regs)
 {
-  clobbers.safe_push (gen_rtx_REG (CCFPmode, FPSR_REG));
-  SET_HARD_REG_BIT (clobbered_regs, FPSR_REG);
+  bool noflags_used = false, nofpsr_used = false;
+
+  for (unsigned i = 0; i < clobbers.length (); )
+    {
+      const_rtx clobbered_reg = clobbers[i];
+
+      if (! REG_P (clobbered_reg))
+	{
+	  ++i;
+	  continue;
+	}
+
+      switch (REGNO (clobbered_reg))
+	{
+	default:             ++i; continue;
+	case NOFLAGS_REGNUM: noflags_used = true; break;
+	case NOFPSR_REGNUM:  nofpsr_used  = true; break;
+	}
+
+      clobbers.unordered_remove (i);
+    }
+
+  if ((noflags_used && TEST_HARD_REG_BIT (clobbered_regs, FLAGS_REG))
+      || (nofpsr_used && TEST_HARD_REG_BIT (clobbered_regs, FPSR_REG)))
+	error ("conflicting clobbers in %<asm%>");
+
+  if (TARGET_DEFAULT_ASM_CLOBBERS && !nofpsr_used
+      && !TEST_HARD_REG_BIT (clobbered_regs, FPSR_REG))
+    {
+      clobbers.safe_push (gen_rtx_REG (CCFPmode, FPSR_REG));
+      SET_HARD_REG_BIT (clobbered_regs, FPSR_REG);
+    }
 
   bool saw_asm_flag = false;
 
@@ -48395,6 +48430,11 @@  ix86_md_asm_adjust (vec<rtx> &outputs, v
       const char *con = constraints[i];
       if (strncmp (con, "=@cc", 4) != 0)
 	continue;
+      if (noflags_used)
+	{
+	  error ("%<asm%> flag output conflicts with clobbers");
+	  continue;
+	}
       con += 4;
       if (strchr (con, ',') != NULL)
 	{
@@ -48530,13 +48570,16 @@  ix86_md_asm_adjust (vec<rtx> &outputs, v
 
   if (saw_asm_flag)
     return seq;
-  else
+
+  if (TARGET_DEFAULT_ASM_CLOBBERS && !noflags_used
+      && !TEST_HARD_REG_BIT (clobbered_regs, FLAGS_REG))
     {
       /* If we had no asm flag outputs, clobber the flags.  */
       clobbers.safe_push (gen_rtx_REG (CCmode, FLAGS_REG));
       SET_HARD_REG_BIT (clobbered_regs, FLAGS_REG);
-      return NULL;
     }
+
+  return NULL;
 }
 
 /* Implements target vector targetm.asm.encode_section_info.  */
--- 2016-06-30/gcc/config/i386/i386.h
+++ 2016-06-30/gcc/config/i386/i386.h
@@ -1304,6 +1304,11 @@  extern const char *host_detect_local_cpu
       : REAL_PIC_OFFSET_TABLE_REGNUM)					\
    : INVALID_REGNUM)
 
+/* Fake register numbers to be used as "inverse" asm() clobber specifiers.
+   Any negative numbers below the range used by decode_reg_name () will do.  */
+#define NOFLAGS_REGNUM (-127)
+#define NOFPSR_REGNUM (-126)
+
 #define GOT_SYMBOL_NAME "_GLOBAL_OFFSET_TABLE_"
 
 /* This is overridden by <cygwin.h>.  */
@@ -2141,7 +2146,11 @@  do {							\
   { "zmm16", 53}, { "zmm17", 54}, { "zmm18", 55}, { "zmm19", 56},	\
   { "zmm20", 57}, { "zmm21", 58}, { "zmm22", 59}, { "zmm23", 60},	\
   { "zmm24", 61}, { "zmm25", 62}, { "zmm26", 63}, { "zmm27", 64},	\
-  { "zmm28", 65}, { "zmm29", 66}, { "zmm30", 67}, { "zmm31", 68} }
+  { "zmm28", 65}, { "zmm29", 66}, { "zmm30", 67}, { "zmm31", 68},	\
+  { "!flags", NOFLAGS_REGNUM }, { "!fpsr", NOFPSR_REGNUM } }
+
+#define OVERLAPPING_REGISTER_NAMES \
+{ { "cc", FLAGS_REG, 2 }, { "!cc", NOFLAGS_REGNUM, 2 } }
 
 /* Note we are omitting these since currently I don't know how
 to get gcc to use these, since they want the same but different
--- 2016-06-30/gcc/config/i386/i386.opt
+++ 2016-06-30/gcc/config/i386/i386.opt
@@ -304,6 +304,10 @@  Enum(pmode) String(long) Value(PMODE_DI)
 mcpu=
 Target RejectNegative Joined Undocumented Alias(mtune=) Warn(%<-mcpu=%> is deprecated; use %<-mtune=%> or %<-march=%> instead)
 
+mdefault-asm-clobbers
+Target RejectNegative Report InverseMask(NO_DEFAULT_ASM_CLOBBERS, DEFAULT_ASM_CLOBBERS) Undocumented Save
+Attach compatibility clobbers (flags and fpsr) to every asm().
+
 mfancy-math-387
 Target RejectNegative Report InverseMask(NO_FANCY_MATH_387, USE_FANCY_MATH_387) Save
 Generate sin, cos, sqrt for FPU.
@@ -372,6 +376,10 @@  Use native (MS) bitfield layout.
 mno-align-stringops
 Target RejectNegative Report Mask(NO_ALIGN_STRINGOPS) Undocumented Save
 
+mno-default-asm-clobbers
+Target RejectNegative Report Mask(NO_DEFAULT_ASM_CLOBBERS) Save
+Don't attach compatibility clobbers (flags and fpsr) to every asm().
+
 mno-fancy-math-387
 Target RejectNegative Report Mask(NO_FANCY_MATH_387) Undocumented Save
 
--- 2016-06-30/gcc/testsuite/gcc.target/i386/20060218-1.c
+++ 2016-06-30/gcc/testsuite/gcc.target/i386/20060218-1.c
@@ -3,6 +3,6 @@ 
 void
 foo (void)
 {
-  register int cc __asm ("cc"); /* { dg-error "invalid register name" } */
+  register int cc __asm ("cc"); /* { dg-error "register specified .* not general enough" } */
   __asm ("" : : "r" (cc) : "cc");
 }
--- 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr1.c
+++ 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr1.c
@@ -0,0 +1,32 @@ 
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+void test(void)
+{
+  int v;
+
+  asm ("" ::: "cc");
+  asm ("" ::: "!cc");
+  asm ("" ::: "flags");
+  asm ("" ::: "!flags");
+  asm ("" ::: "fpsr");
+  asm ("" ::: "!fpsr");
+
+  asm ("" ::: "cc", "!cc");		/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "flags", "!flags");	/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "fpsr", "!fpsr");		/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "flags", "!cc");		/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "cc", "!flags");		/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "fpsr", "!cc");		/* { dg-error "conflicting clobbers" } */
+  asm ("" ::: "cc", "!fpsr");		/* { dg-error "conflicting clobbers" } */
+
+#if 0 /* These right now are internal compiler errors.  */
+  asm ("" : "=@ccz" (v) :: "cc");	/* { dg-bogus "clobber conflict with output" } */
+  asm ("" : "=@ccz" (v) :: "flags");	/* { dg-bogus "clobber conflict with output" } */
+#endif
+  asm ("" : "=@ccz" (v) :: "!cc");	/* { dg-error "output conflicts with clobbers" } */
+  asm ("" : "=@ccz" (v) :: "!flags");	/* { dg-error "output conflicts with clobbers" } */
+
+  asm ("" ::: "flags", "!fpsr");
+  asm ("" ::: "fpsr", "!flags");
+}
--- 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr2.c
+++ 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr2.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdump-rtl-expand" } */
+
+void test(void)
+{
+  asm ("" ::: "!cc");
+  asm ("" ::: "!fpsr", "!flags");
+}
+
+/* { dg-final { scan-rtl-dump-not "clobber" "expand" } } */
--- 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr3.c
+++ 2016-06-30/gcc/testsuite/gcc.target/i386/invclbr3.c
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdump-rtl-expand" } */
+
+void test(void)
+{
+  asm ("" ::: "!cc");
+  asm ("" ::: "!flags");
+  asm ("" ::: "!fpsr");
+}
+
+/* { dg-final { scan-rtl-dump-times "\\(clobber \\(reg\[^()\]* fpsr\\)\\)" 1 "expand" } } */
+/* { dg-final { scan-rtl-dump-times "\\(clobber \\(reg\[^()\]* flags\\)\\)" 1 "expand" } } */
--- 2016-06-30/gcc/varasm.c
+++ 2016-06-30/gcc/varasm.c
@@ -943,7 +943,7 @@  decode_reg_name_and_count (const char *a
 	for (i = 0; i < (int) ARRAY_SIZE (table); i++)
 	  if (table[i].name[0]
 	      && ! strcmp (asmspec, table[i].name)
-	      && reg_names[table[i].number][0])
+	      && (table[i].number < 0 || reg_names[table[i].number][0]))
 	    return table[i].number;
       }
 #endif /* ADDITIONAL_REGISTER_NAMES */