Re: RFA: [VAX] SUBREG of MEM with a mode dependent address

[Matt Thomas <matt at 3am-software dot com>]

On May 30, 2014, at 10:39 AM, Jeff Law <law@redhat.com> wrote:

> On 05/25/14 18:19, Matt Thomas wrote:
>> 
>> But even if  movhi is a define_expand, as far as I can tell there's
>> isn't enough info to know whether that is possible.  At that time,
>> how can I tell that operands[0] will be a hard reg or operands[1]
>> will be subreg of a mode dependent memory access?
> At that time, you can't know those things.  Not even close ;-)  You certainly don't want to try and rewrite the insn to just use SImode. This is all an indication something has gone wrong elsewhere and this would just paper over the problem.
> 
>> 
>> I've tried using secondary_reload and it called called with
>> 
>> (subreg:HI (reg:SI 113 [ MEM[base: _154, offset: 0B] ]) 0)
>> 
>> but it dies in change_address_1 before invoking the code returned in
>> sri.
> I suspect if you dig deep enough, you can make a secondary reload do what you want.  It's just amazingly painful.
> 
> You want to allocate an SImode temporary, do the load of the SI memory location into that SImode temporary, then (subreg:SI (tempreg:SI)). Your best bet is going to be to look at how some other ports handle their secondary reloads.  But I warn you, it's going to be painful.

Doesn't work because the assert fires before the secondary reload takes place.

In expr.c:convert_mode there is code that would seem to prevent this:

  /* For truncation, usually we can just refer to FROM in a narrower mode.  */
  if (GET_MODE_BITSIZE (to_mode) < GET_MODE_BITSIZE (from_mode)
      && TRULY_NOOP_TRUNCATION_MODES_P (to_mode, from_mode))
    {
      if (!((MEM_P (from)
             && ! MEM_VOLATILE_P (from)
             && direct_load[(int) to_mode]
             && ! mode_dependent_address_p (XEXP (from, 0),
                                            MEM_ADDR_SPACE (from)))
            || REG_P (from)
            || GET_CODE (from) == SUBREG))
        from = force_reg (from_mode, from);
      if (REG_P (from) && REGNO (from) < FIRST_PSEUDO_REGISTER
          && ! HARD_REGNO_MODE_OK (REGNO (from), to_mode))
        from = copy_to_reg (from);
      emit_move_insn (to, gen_lowpart (to_mode, from));
      return;
    }

but from at that point is just

(mem:SI (reg:SI 112 [ D.118399 ]) [4 MEM[base: _154, offset: 0B]+0 S4 A32])

So there is not enough information for mode_dependent_address_p to return true.

>> 
>> I've tracked this down to reload replacing (reg:SI 113) with
>> reg_equiv_mem (133) in the rtx.  However, it doesn't verify the rtx
>> is actually valid.  I added a gcc_assert to trap this and got:
> Right.  reload will make that replacement and it's not going to do any verification at that point.  Verification would have happened earlier.

See above.  If anywhere, that is where it would have been done.

> You have to look at the beginning of the main reload loop and poke at that for a while:
> 
>     /* For each pseudo register that has an equivalent location defined,
>         try to eliminate any eliminable registers (such as the frame pointer)
>         assuming initial offsets for the replacement register, which
>         is the normal case.
> 
>         If the resulting location is directly addressable, substitute
>         the MEM we just got directly for the old REG.
> 
>         If it is not addressable but is a constant or the sum of a hard reg
>         and constant, it is probably not addressable because the constant is
>         out of range, in that case record the address; we will generate
>         hairy code to compute the address in a register each time it is
>         needed.  Similarly if it is a hard register, but one that is not
>         valid as an address register.
> 
>         If the location is not addressable, but does not have one of the
>         above forms, assign a stack slot.  We have to do this to avoid the
>         potential of producing lots of reloads if, e.g., a location involves
>         a pseudo that didn't get a hard register and has an equivalent memory
>         location that also involves a pseudo that didn't get a hard register.
> 
>         Perhaps at some point we will improve reload_when_needed handling
>         so this problem goes away.  But that's very hairy.  */

I found a simplier solution.  It seemed to me that reload_inner_reg_of_subreg
was the right place to make this happen.  The following diff (to gcc 4.8.3)
fixes the problem:

diff -u -p -r1.3 reload.c
--- gcc/reload.c        1 Mar 2014 08:58:29 -0000       1.3
+++ gcc/reload.c        3 Jun 2014 17:24:27 -0000
@@ -846,6 +846,7 @@ static bool
 reload_inner_reg_of_subreg (rtx x, enum machine_mode mode, bool output)
 {
   rtx inner;
+  int regno;
 
   /* Only SUBREGs are problematical.  */
   if (GET_CODE (x) != SUBREG)
@@ -857,10 +858,20 @@ reload_inner_reg_of_subreg (rtx x, enum 
   if (CONSTANT_P (inner) || GET_CODE (inner) == PLUS)
     return true;
 
-  /* If INNER is not a hard register, then INNER will not need reloading.  */
-  if (!(REG_P (inner) && HARD_REGISTER_P (inner)))
+  /* If INNER is not a register, then INNER will not need reloading.  */
+  if (!REG_P (inner))
     return false;
 
+  regno = REGNO (inner);
+
+  /* If INNER is not a hard register, then INNER will not need reloading
+     unless it's a mode dependent memory reference.  */
+  if (regno >= FIRST_PSEUDO_REGISTER)
+    return !output
+          && reg_equiv_mem (regno) != 0
+          && mode_dependent_address_p (XEXP (reg_equiv_mem (regno), 0),
+                                       MEM_ADDR_SPACE (reg_equiv_mem (regno)));
+
   /* If INNER is not ok for MODE, then INNER will need reloading.  */
   if (!HARD_REGNO_MODE_OK (subreg_regno (x), mode))
     return true;
@@ -1142,7 +1153,7 @@ push_reload (rtx in, rtx out, rtx *inloc
 
   if (in != 0 && reload_inner_reg_of_subreg (in, inmode, false))
     {
-      if (REG_P (SUBREG_REG (in)))
+      if (REG_P (SUBREG_REG (in)) && HARD_REGISTER_P (SUBREG_REG (in)))
        subreg_in_class
          = find_valid_class (inmode, GET_MODE (SUBREG_REG (in)),
                              subreg_regno_offset (REGNO (SUBREG_REG (in)),
@@ -1150,7 +1161,8 @@ push_reload (rtx in, rtx out, rtx *inloc
                                                   SUBREG_BYTE (in),
                                                   GET_MODE (in)),
                              REGNO (SUBREG_REG (in)));
-      else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+      else if (REG_P (SUBREG_REG (in))
+               || GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
        subreg_in_class = find_valid_class_1 (inmode,
                                              GET_MODE (SUBREG_REG (in)),
                                              rclass);

Give that reload_inner_reg_of_subreg returns true for CONSTANT_P or PLUS,
I have to think that last else shouldn't have the if (...) and just be
a naked else.