RFA: fix rtl-optimization/56833

Quoting Eric Botcazou <ebotcazou@adacore.com>:

>> The problem was that we had some optimzations added to the
>> reload_cse_move2add pass that would attempt transformations with
>> multi-hard-register registers, without keeping track of the validity of the
>> values in all hard registers involved.
>
> That's not clear to me: for example in move2add_note_store, we reset the info
> about any multi hard-register; moveover 5 arrays are supposed to be   
> maintained
> for each hard-register:
>
>   for (i = FIRST_PSEUDO_REGISTER - 1; i >= 0; i--)
>     {
>       reg_set_luid[i] = 0;
>       reg_offset[i] = 0;
>       reg_base_reg[i] = 0;
>       reg_symbol_ref[i] = NULL_RTX;
>       reg_mode[i] = VOIDmode;
>     }
> so I'm not sure whether we really properly handle multi hard-registers.

Initializing all the values is nice to have defined contents, but
the values in reg_offset[i] / reg_base_reg[i] / reg_symbol_ref[i]
just tell what's in these registers if they are valid, not if they
are valid.
But I can see that there could be a problem with an earlier value
that used to be valid in a multi-hard-register sub register to be
considered to be still valid.
Setting the mode of every constituent register but the first one
(which has the new value recorded) to VOIDmode at the same time
as updating reg_set_luid should be sufficent to address this issue.

> So
> what about explicitly punting for multi hard-registers in   
> reload_cse_move2add?
> Do you think that this would pessimize too much in practice?

The pass was originally written with word_mode == Pmode targets like
the SH in mind, where multi-hard-register values are uninteresting.

But for targets like the avr, most or all of the interesting values
will be in multi-hard-register registers.
2013-05-22  Joern Rennecke <joern.rennecke@embecosm.com>

	PR rtl-optimization/56833
	* postreload.c (move2add_use_add2_insn): Update reg_set_luid
	and reg_mode for all affected hard registers.
	(move2add_use_add3_insn): Likewise.  Check that all source hard
	regs have been set by the same set.
	(reload_cse_move2add): Before concluding that we have a pre-existing
	value, check that all destination hard registers have been set by the
	same set.

> But I can see that there could be a problem with an earlier value
> that used to be valid in a multi-hard-register sub register to be
> considered to be still valid.
> Setting the mode of every constituent register but the first one
> (which has the new value recorded) to VOIDmode at the same time
> as updating reg_set_luid should be sufficent to address this issue.

Agreed.

> The pass was originally written with word_mode == Pmode targets like
> the SH in mind, where multi-hard-register values are uninteresting.
> 
> But for targets like the avr, most or all of the interesting values
> will be in multi-hard-register registers.

The patch is OK on principle, but can you factor out the common code?  The 
endings of move2add_use_add2_insn and move2add_use_add3_insn are identical so 
it would be nice to have e.g. a record_reg_value helper function and call it 
from there.  Similarly, the 3 new checks look strictly identical.

Quoting Eric Botcazou <ebotcazou@adacore.com>:

> The patch is OK on principle, but can you factor out the common code?  The
> endings of move2add_use_add2_insn and move2add_use_add3_insn are identical so
> it would be nice to have e.g. a record_reg_value helper function and call it
> from there.  Similarly, the 3 new checks look strictly identical.

Looking into sharing the code with sites that perform essentially the same
function but look somewhat different, I see there's a problem with using
only reg_set_luid to indicate the consistency of a multi-hard-reg-value
in these other contexts.
For values that are use a base register, the reg_set_luid is the same as
for the base register; for constants, it is the same for all constants
set since the last label.

Say we have reg size 8 bit, base r0, and then
(set reg:HI 2 (plus:SI (reg:HI 0) (const_int 500))
...
(set reg:HI 3 (plus:SI (reg:HI 0) (const_int 500))

Now how do we tell that the value in r2 is no longer valid?

As the example shows, trying to replicate the recorded value across all hard
regs is pointless, as we still need to make sure that we have a still-valid
start register.
OTOH, this ties in nicely with setting the mode of subsequent registers
to VOIDmode.  We can verify the mode to make sure there was no more recent
set of any constituent register.  The check of the extra luids thus becomes
superflous, as becomes the set.
This logic relies on multi-hard register regs to be allocated contigously...
But if we'd want to change that, there'd be a lot more code that would
need changing.