Patchwork [RFC,i386] : Allow SUBREG_PROMOTED_UNSIGNED_P subregs in address

login
register
mail settings
Submitter Uros Bizjak
Date July 20, 2011, 4:41 p.m.
Message ID <CAFULd4bmNZZAzJmJdW6SyRj4k-OteJkMnpCf9-p7dF624drXfg@mail.gmail.com>
Download mbox | patch
Permalink /patch/105760/
State New
Headers show

Comments

Uros Bizjak - July 20, 2011, 4:41 p.m.
Hello!

Attached RFC patch buids on recent x86 improvements in
ix86_legitimate_address_p/ix86_decompose_address functions. With this
patch, we allow SUBREGs with register_no_elim_operand registers in
base, index and PLUS chains. As the consequence, we can relax the
condition that rejects SUBREGs, wider than word mode due to possible
spill failures with double-word SUBREGs. register_no_elim_operand
together with REG_OK_FOR_BASE/REG_OK_FOR_INDEX predicates guarantee,
that there will be no spills since multi-word registers that satisfy
these predicates can easily be decomposed to satisfy SUBREGs.

Another improvement is introduction of SUBREG_PROMOTED_UNSIGNED_P
predicate in the same area to handle paradoxical SUBREGs. When SUBREG
satisfies this predicate, the compiler guarantees, that excess bits
are zero (see the documentation). This is exaclty what we want in
registers that form the address. In theory, we can allow registers of
any integer mode here, but in practice only DImode subreg of SImode
register is important.

IMO, the first optimization is important on i686, while the second is
important on x32 (lots of paradoxical subregs in addresses due to
Pmode != ptr_mode), but they are also nice to have on x86_64.

Any thoughts?

2011-07-20  Uros Bizjak  <ubizjak@gmail.com>

	* config/i386/i386.c (ix86_decompose_address): Also allow promoted
	paradoxical subregs in base and PLUS chains.  Allow only paradoxical
	subregs and subregs of DImode hard registers in subregs of index.
	(ix86_legitimate_address_p): Allow subregs of base and index to span
	more than a word.  Assert that subregs of base and index satisfy
	SUBREG_PROMOTED_UNSIGNED_P or register_no_elim_operand predicates.

Patch was bootstrapped and is currently regression testing on
x86_64-pc-linux-gnu {,-m32}.

Uros.
Richard Henderson - July 20, 2011, 5:11 p.m.
On 07/20/2011 09:41 AM, Uros Bizjak wrote:
> 	* config/i386/i386.c (ix86_decompose_address): Also allow promoted
> 	paradoxical subregs in base and PLUS chains.  Allow only paradoxical
> 	subregs and subregs of DImode hard registers in subregs of index.
> 	(ix86_legitimate_address_p): Allow subregs of base and index to span
> 	more than a word.  Assert that subregs of base and index satisfy
> 	SUBREG_PROMOTED_UNSIGNED_P or register_no_elim_operand predicates.

The patch looks good.

It seems like one other check is required: that the mode of base and
index are the same.  We've been accepting either SImode or DImode.
As long as they're both the same we get either

	mov	(%rax, %rdx, 4), %ebx
	mov	(%eax, %edx, 4), %ebx

which are both valid insns (without and with addr32 prefix).  Whereas

	mov	(%rax, %edx, 4), %ebx

will correctly be rejected by the assembler.


r~
Eric Botcazou - July 20, 2011, 6:04 p.m.
> Another improvement is introduction of SUBREG_PROMOTED_UNSIGNED_P
> predicate in the same area to handle paradoxical SUBREGs. When SUBREG
> satisfies this predicate, the compiler guarantees, that excess bits
> are zero (see the documentation). This is exaclty what we want in
> registers that form the address. In theory, we can allow registers of
> any integer mode here, but in practice only DImode subreg of SImode
> register is important.

Note that SUBREG_PROMOTED_UNSIGNED_P wasn't designed for paradoxical subregs, 
but for regular subregs (typically of word-sized objects).  You should check 
that the ones created for x32 (because of POINTERS_EXTEND_UNSIGNED I guess) 
are legitimate.
Uros Bizjak - July 20, 2011, 7:46 p.m.
On Wed, Jul 20, 2011 at 8:04 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Another improvement is introduction of SUBREG_PROMOTED_UNSIGNED_P
>> predicate in the same area to handle paradoxical SUBREGs. When SUBREG
>> satisfies this predicate, the compiler guarantees, that excess bits
>> are zero (see the documentation). This is exaclty what we want in
>> registers that form the address. In theory, we can allow registers of
>> any integer mode here, but in practice only DImode subreg of SImode
>> register is important.
>
> Note that SUBREG_PROMOTED_UNSIGNED_P wasn't designed for paradoxical subregs,
> but for regular subregs (typically of word-sized objects).  You should check
> that the ones created for x32 (because of POINTERS_EXTEND_UNSIGNED I guess)
> are legitimate.

Hm, the documentation says:

    Paradoxical subregs
          When M1 is strictly wider than M2, the `subreg' expression is
          called "paradoxical".  The canonical test for this class of
          `subreg' is:

               GET_MODE_SIZE (M1) > GET_MODE_SIZE (M2)

          Paradoxical `subreg's can be used as both lvalues and rvalues.
          When used as an lvalue, the low-order bits of the source value
          are stored in REG and the high-order bits are discarded.
          When used as an rvalue, the low-order bits of the `subreg' are
          taken from REG while the high-order bits may or may not be
          defined.

          The high-order bits of rvalues are in the following
          circumstances:

             * `subreg's of `mem' When M2 is smaller than a word, the
               macro `LOAD_EXTEND_OP', can control how the high-order
               bits are defined.

             * `subreg' of `reg's The upper bits are defined when
               `SUBREG_PROMOTED_VAR_P' is true.
               `SUBREG_PROMOTED_UNSIGNED_P' describes what the upper
               bits hold.  Such subregs usually represent local
               variables, register variables and parameter pseudo
               variables that have been promoted to a wider mode.

I have looked at example in rs6000.c, the only target that uses
SUBREG_PROMOTED_UNSIGNED_P. Looking at other sources, S_P_U_P is used
in conjunction with SUBREG_PROMOTED_VAR. It looks to me that using the
combination should be OK to determine if subreg is correct.

Uros.

Patch

Index: i386.c
===================================================================
--- i386.c	(revision 176512)
+++ i386.c	(working copy)
@@ -11089,8 +11089,10 @@  ix86_decompose_address (rtx addr, struct
     base = addr;
   else if (GET_CODE (addr) == SUBREG)
     {
-      /* Allow only subregs of DImode hard regs.  */
-      if (register_no_elim_operand (SUBREG_REG (addr), DImode))
+      /* Allow only promoted paradoxical subregs
+	 or subregs of DImode hard regs.  */
+      if (SUBREG_PROMOTED_UNSIGNED_P (addr)
+	  || register_no_elim_operand (SUBREG_REG (addr), DImode))
 	base = addr;
       else
 	return 0;
@@ -11148,8 +11150,10 @@  ix86_decompose_address (rtx addr, struct
 	      break;
 
 	    case SUBREG:
-	      /* Allow only subregs of DImode hard regs in PLUS chains.  */
-	      if (!register_no_elim_operand (SUBREG_REG (op), DImode))
+	      /* Allow only promoted paradoxical subregs
+		 or subregs of DImode hard regs in PLUS chains.  */
+	      if (!(SUBREG_PROMOTED_UNSIGNED_P (op)
+		    || register_no_elim_operand (SUBREG_REG (op), DImode)))
 		return 0;
 	      /* FALLTHRU */
 
@@ -11197,6 +11201,18 @@  ix86_decompose_address (rtx addr, struct
   else
     disp = addr;			/* displacement */
 
+  if (index)
+    {
+      if (REG_P (index))
+	;
+      /* Allow only promoted paradoxical subregs
+	 or subregs of DImode hard regs.  */
+      else if (GET_CODE (index) == SUBREG
+	       && !(SUBREG_PROMOTED_UNSIGNED_P (index)
+		    || register_no_elim_operand (SUBREG_REG (index), DImode)))
+	return 0;
+    }
+
   /* Extract the integral value of scale.  */
   if (scale_rtx)
     {
@@ -11630,11 +11646,7 @@  ix86_legitimate_address_p (enum machine_
   disp = parts.disp;
   scale = parts.scale;
 
-  /* Validate base register.
-
-     Don't allow SUBREG's that span more than a word here.  It can lead to spill
-     failures when the base is one word out of a two word structure, which is
-     represented internally as a DImode int.  */
+  /* Validate base register.  */
 
   if (base)
     {
@@ -11642,11 +11654,12 @@  ix86_legitimate_address_p (enum machine_
 
       if (REG_P (base))
   	reg = base;
-      else if (GET_CODE (base) == SUBREG
-	       && REG_P (SUBREG_REG (base))
-	       && GET_MODE_SIZE (GET_MODE (SUBREG_REG (base)))
-		  <= UNITS_PER_WORD)
-  	reg = SUBREG_REG (base);
+      else if (GET_CODE (base) == SUBREG && REG_P (SUBREG_REG (base)))
+	{
+	  reg = SUBREG_REG (base);
+	  gcc_assert (SUBREG_PROMOTED_UNSIGNED_P (base)
+		      || register_no_elim_operand (reg, DImode));
+	}
       else
 	/* Base is not a register.  */
 	return false;
@@ -11660,9 +11673,7 @@  ix86_legitimate_address_p (enum machine_
 	return false;
     }
 
-  /* Validate index register.
-
-     Don't allow SUBREG's that span more than a word here -- same as above.  */
+  /* Validate index register.  */
 
   if (index)
     {
@@ -11670,11 +11681,12 @@  ix86_legitimate_address_p (enum machine_
 
       if (REG_P (index))
   	reg = index;
-      else if (GET_CODE (index) == SUBREG
-	       && REG_P (SUBREG_REG (index))
-	       && GET_MODE_SIZE (GET_MODE (SUBREG_REG (index)))
-		  <= UNITS_PER_WORD)
-  	reg = SUBREG_REG (index);
+      else if (GET_CODE (index) == SUBREG && REG_P (SUBREG_REG (index)))
+	{
+	  reg = SUBREG_REG (index);
+	  gcc_assert (SUBREG_PROMOTED_UNSIGNED_P (index)
+		      || register_no_elim_operand (reg, DImode));
+	}
       else
 	/* Index is not a register.  */
 	return false;