Re: Make m32c build, fix PSImode truncation

[Richard Sandiford <rsandifo at linux dot vnet dot ibm dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> On 04/27/2013 10:39 AM, Richard Sandiford wrote:
>>> Argh, that's unfortunate.  The point of that change was to make
>>> simplify_gen_unary (TRUNCATE, ...) no worse than using a subreg.
>>> Would the equivalent lowpart simplify_gen_subreg call succeed
>>> (return nonnull)?  If so, I think we want truncate to do the same.
>>> 
>>> What simplification is this blocking, and why does it lead to
>>> reload failures?
>>
>> There's an explicit (set (reg:PSI) (truncate:PSI (reg:SI)) insn which
>> currently gets changed to (set (reg:PSI) (subreg:PSI (reg:SI)) during
>> cse1. Reload fails because the subreg gets propagated into a memory
>> address, which requires a class of A_REGS, but A_REGS can only hold
>> PSImode values, not SImode.  This shows that the truncation is not
>> always a no-op: in this case it involves a register move, but there's no
>> way to describe this using TRULY_NOOP_TRUNCATION.
>
> Hmm, but isn't this a reload bug?  We have:
>
> (insn 53 51 54 10 (set (reg:HI 0 r0 [orig:26 D.2817 ] [26])
>         (zero_extend:HI (mem/u/j:QI (plus:PSI (subreg:PSI (reg:SI 44 [ D.2818 ]) 0)
>                     (symbol_ref:PSI ("__clz_tab") [flags 0x40]  <var_decl 0x7f2c253d42f8 __clz_tab>)) [0 __clz_tab S1 A8]))) /home/richards/gcc/HEAD/gcc/libgcc/libgcc2.c:520 115 {zero_extendqihi2}
>      (expr_list:REG_DEAD (reg:SI 44 [ D.2818 ])
>         (nil)))
>
> Reloads for insn # 53
> Reload 0: reload_in (SI) = (reg:SI 44 [ D.2818 ])
>         A_REGS, RELOAD_FOR_OTHER_ADDRESS (opnum = 0)
>         reload_in_reg: (reg:SI 44 [ D.2818 ])
>
> find_reloads_address_1 is reloading the SUBREG_REG rather than the
> SUBREG itself, even though SImode is not valid for BASE_REGS == A_REGS:
>
> 	if (GET_CODE (op0) == SUBREG)
> 	  {
> 	    op0 = SUBREG_REG (op0);
> 	    code0 = GET_CODE (op0);
> 	    if (code0 == REG && REGNO (op0) < FIRST_PSEUDO_REGISTER)
> 	      op0 = gen_rtx_REG (word_mode,
> 				 (REGNO (op0) +
> 				  subreg_regno_offset (REGNO (SUBREG_REG (orig_op0)),
> 						       GET_MODE (SUBREG_REG (orig_op0)),
> 						       SUBREG_BYTE (orig_op0),
> 						       GET_MODE (orig_op0))));
> 	  }
>
> push_reloads would specifically not convert a SUBREG reload to a
> REG reload in this case.  In principle, I think address subregs
> should be handled in the same way.
>
> So is the problem really that (subreg:PSI (reg:SI ...)) isn't a valid
> truncation on m32c?  Without TRULY_NOOP_TRUNCATION, I don't see what
> forces most code to use (truncate:PSI (reg:SI ...)) instead.  Many places
> would call gen_lowpart directly.
>
> Sorry for missing the truncation patterns, I should have grepped more
> than m32c.md.  They look a lot like normal moves though.  Is truncation
> really not a noop, or are the patterns there to work around something
> (probably this :-))?

Even if that's true, I suppose it isn't worth trying to fix such a
sensitive part of reload at this stage.  I think LRA already handles
it correctly.

In the meantime, we could work around the problem by disallowing subregs
in m32c addresses.  I think all non-paradoxical subregs[*] are going to
need a reload anyway, so it should also produce better code.

 [*] Paradoxical subregs imply an address has don't-care bits, so should
     be rare.

FWIW, the proof-of-concept patch below restores the build for me.
I realise it might fail muster on style grounds though.

Richard

gcc/
	* config/m32c/m32c.c (address_pattern_p): New variable.
	(encode_pattern_1): Include subregs address_pattern_p.
	(encode_pattern): Add address_p parameter.
	(m32c_legitimate_address_p): Update accordingly.

Index: gcc/config/m32c/m32c.c
===================================================================
--- gcc/config/m32c/m32c.c	2013-04-29 14:07:50.000000000 +0100
+++ gcc/config/m32c/m32c.c	2013-04-29 14:07:51.207987093 +0100
@@ -113,6 +113,7 @@ static int class_contents[LIM_REG_CLASSE
 /* These are all to support encode_pattern().  */
 static char pattern[30], *patternp;
 static GTY(()) rtx patternr[30];
+static bool address_pattern_p;
 #define RTX_IS(x) (streq (pattern, x))
 
 /* Some macros to simplify the logic throughout this file.  */
@@ -166,8 +167,9 @@ encode_pattern_1 (rtx x)
       *patternp++ = 'r';
       break;
     case SUBREG:
-      if (GET_MODE_SIZE (GET_MODE (x)) !=
-	  GET_MODE_SIZE (GET_MODE (XEXP (x, 0))))
+      if (address_pattern_p
+	  || (GET_MODE_SIZE (GET_MODE (x))
+	      != GET_MODE_SIZE (GET_MODE (XEXP (x, 0)))))
 	*patternp++ = 'S';
       encode_pattern_1 (XEXP (x, 0));
       break;
@@ -254,9 +256,10 @@ encode_pattern_1 (rtx x)
 }
 
 static void
-encode_pattern (rtx x)
+encode_pattern (rtx x, bool address_p = false)
 {
   patternp = pattern;
+  address_pattern_p = address_p;
   encode_pattern_1 (x);
   *patternp = 0;
 }
@@ -1684,7 +1687,7 @@ m32c_legitimate_address_p (enum machine_
     }
 #endif
 
-  encode_pattern (x);
+  encode_pattern (x, true);
   if (RTX_IS ("r"))
     {
       /* Most indexable registers can be used without displacements,