Re: RFC: reloading sums

[Joern Rennecke <joern dot rennecke at superh dot com>]

Michael Matz wrote:
> Strictly speaking this is OK, as register 13 will not be used anymore,
> except for that reload.  The problem is rather, that reload doesn't see
> that also the constant is not legitimate.  Why is no reload for that
> constant generated?

find_reloads_address_part is responsible for reloading constants in
addresses.

  else if (GET_CODE (x) == PLUS
           && CONSTANT_P (XEXP (x, 1))
           && (! LEGITIMATE_CONSTANT_P (XEXP (x, 1))
               || PREFERRED_RELOAD_CLASS (XEXP (x, 1), class) == NO_REGS))
    {
      rtx tem;

      tem = force_const_mem (GET_MODE (x), XEXP (x, 1));
      x = gen_rtx_PLUS (GET_MODE (x), XEXP (x, 0), tem);
      find_reloads_address (mode, &tem, XEXP (tem, 0), &XEXP (tem, 0),
                            opnum, type, ind_levels, 0);
    }

Note that LEGITIMATE_CONSTANT_P accepts the constant because it is valid
in a constant load, and PREFERRED_RELOAD_CLASS is supposed to be used
for optimizations only.

gen_reload uses this test to determine if it should swap the operands to
an add when it emits its two-insn sequence:

      code = (int) add_optab->handlers[(int) GET_MODE (out)].insn_code;

      if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == SUBREG
          || (GET_CODE (op1) == REG
              && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
          || (code != CODE_FOR_nothing
              && ! ((*insn_data[code].operand[2].predicate)
                    (op1, insn_data[code].operand[2].mode))))
        tem = op0, op0 = op1, op1 = tem;

Likewise, it would make sense to make finds_reloads_address_part push a reload
for constants in a PLUS that are not accepted by the addxx3 pattern, yet are
accepted as legitimate constants.  So, that would make an conceptually cleaner
patch.
The downside is that an instruction that uses three pseudos that ended up
in far-away stack slots can end up with three more reloads, thus necessitating
to re-think our upper bounds on the number of reloads we handle.  We also have
to contemplate an extra type for every flavour of reload (we could use
RELOAD_FOR_INPADDR_ADDRESS etc. and RELOAD_OTHER, but then we get worse code,
and also risk running out of reload registers).
Moreover, the constant is most likely to be useless for reload inheritance;
except for purely random number matches, the only use for the constant will
be to calculate the sum, so it is better to inherit the sum in the first place.
Thus, we should change the way reload registers are reused, instead of using
them in round-robin fashion we should use a FIFO with the possibility to put
certain registers at the top of the queue (or we could keep a list of registers
to be re-used first).

If you want to write, test and debug such a patch for 3.5, that would be
kind of neat, but I won't hold my breath.
In the meantime, we have a a bug to fix, and disabling an optimization under
circumstances when we are almost certain the optimization would be incorrect
is a save way to do it.
 
> > (insn 1481 1006 1482 57 (set (reg:SI 13 r13)
> >         (const_int 136 [0x88])) -1 (nil)
> >     (nil))
> 
> It's this insn which seems to be added by your particular addsi3
> instruction which breaks everything.  Not reload.

The sh addsi3 expander does not accept this constant when compiling for
-m5-compact.  The constant load is generated by gen_reload.

> > with rld[n_reloads].in .  That is fine as long as the add can be done
> > with a single insn, but when the constant is so large that it has to be
> > reloaded into a register first, that clobbers the index.
> 
> Not necessarily.  The real condition is "if the add can be done without
> clobbering it's inputs".  Well, such an add should be producable anyway.

How would you produce such an add?
	
-- 
--------------------------
SuperH (UK) Ltd.
2410 Aztec West / Almondsbury / BRISTOL / BS32 4QX
T:+44 1454 465658