Richard Sandiford <rdsandif...@googlemail.com> writes:
> Bernd Schmidt <ber...@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,
