diff mbox

RFA: Fix crx --enable-werror-always build

Message ID 4CE6E1AC.4070309@redhat.com
State New
Headers show

Commit Message

Richard Henderson Nov. 19, 2010, 8:44 p.m. UTC 
On 11/19/2010 11:21 AM, Joern Rennecke wrote:
> Quoting Richard Henderson <rth@redhat.com>:
> 
>> On 11/12/2010 08:22 AM, Joern Rennecke wrote:
>>> +  gcc_assert (REGNO (reg) != CC_REGNUM);
>>
>> This is the only change I'm unsure about.  I wonder if instead
>> the previous IF was supposed to check REG instead of ADDR_REG.
> 
> That seems unlikely, because then the
> 
>            && GET_MODE_SIZE (GET_MODE (SUBREG_REG (addr_reg)))
>            <= UNITS_PER_WORD))
> 
> check would be redundant.

True. 

Comparing this to the i386 legitimate address predicate I see

      if ((strict && ! REG_OK_FOR_BASE_STRICT_P (reg))
          || (! strict && ! REG_OK_FOR_BASE_NONSTRICT_P (reg)))
        /* Base is not valid.  */
        return false;

In the crx case, the un-wrapping of the subreg isn't saved back
into the crx_address structure, and so the subreg will make it to 

      if (address.base && !REGNO_OK_FOR_BASE_P (REGNO (address.base)))

which ought to ICE due to REGNO applied to SUBREG.  Which probably
will never happen, since this only happens for strict, and I expect
that subregs of hard regs are resolved by now.

That said, a more correct patch seems like it would be like this:


r~

Comments

Joern Rennecke Nov. 19, 2010, 9:33 p.m. UTC | #1 
Quoting Richard Henderson <rth@redhat.com>:

> In the crx case, the un-wrapping of the subreg isn't saved back
> into the crx_address structure, and so the subreg will make it to
>
>       if (address.base && !REGNO_OK_FOR_BASE_P (REGNO (address.base)))
>
> which ought to ICE due to REGNO applied to SUBREG.  Which probably
> will never happen, since this only happens for strict, and I expect
> that subregs of hard regs are resolved by now.

cleanup_subreg_operands runs at the end of reload, but during reload you
might still see subregs.

> That said, a more correct patch seems like it would be like this:

Yes, that makes sense.
Although the one thing that stayed the same for crx_addr_reg_p - its name -
doesn't fit as well anymore.
diff mbox

Patch

diff --git a/gcc/config/crx/crx.c b/gcc/config/crx/crx.c
index 5fc963a..35177a8 100644
--- a/gcc/config/crx/crx.c
+++ b/gcc/config/crx/crx.c
@@ -590,30 +590,21 @@  crx_function_arg_regno_p (int n)
  * Scaled index		--> reg + reg | 22-bit disp. + reg + reg |
  *			    22-disp. + reg + reg + (2 | 4 | 8) */
 
-static int crx_addr_reg_p (rtx addr_reg)
+static rtx
+crx_addr_reg_p (rtx addr_reg)
 {
-  rtx reg;
+  if (GET_MODE (addr_reg) != Pmode)
+    return NULL_RTX;
 
   if (REG_P (addr_reg))
-    {
-      reg = addr_reg;
-    }
-  else if ((GET_CODE (addr_reg) == SUBREG
+    return addr_reg;
+  else if (GET_CODE (addr_reg) == SUBREG
 	   && REG_P (SUBREG_REG (addr_reg))
-	   && GET_MODE_SIZE (GET_MODE (SUBREG_REG (addr_reg)))
-	   <= UNITS_PER_WORD))
-    {
-      reg = SUBREG_REG (addr_reg);
-    }
+	   && (GET_MODE_SIZE (GET_MODE (SUBREG_REG (addr_reg)))
+	       <= UNITS_PER_WORD))
+    return SUBREG_REG (addr_reg);
   else
-    return FALSE;
-
-  if (GET_MODE (addr_reg) != Pmode)
-    {
-      return FALSE;
-    }
-
-  return TRUE;
+    return NULL_RTX;
 }
 
 enum crx_addrtype
@@ -752,8 +743,18 @@  crx_decompose_address (rtx addr, struct crx_address *out)
       return CRX_INVALID;
     }
 
-  if (base && !crx_addr_reg_p (base)) return CRX_INVALID;
-  if (index && !crx_addr_reg_p (index)) return CRX_INVALID;
+  if (base)
+    {
+      base = crx_addr_reg_p (base);
+      if (!base)
+	return CRX_INVALID;
+    }
+  if (index)
+    {
+      index = crx_addr_reg_p (index);
+      if (!index)
+	return CRX_INVALID;
+    }
   
   out->base = base;
   out->index = index;