Hi Segher,

Thanks for the review.

On Wed, 12 Dec 2018 19:47:53 -0600
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> The unused bits in a MODE_PARTIAL_INT value are undefined, so nonzero_bits
> isn't valid for conversion in either direction.
>
> And *which* bits are undefined isn't defined anywhere either, so we cannot
> convert to/from smaller MODE_INT modes, either.

Can't we use the last_set_nonzero_bits if last_set_mode was MODE_INT and the
current mode is MODE_PARTIAL_INT? As long as the current mode bitsize is less
than the bitsize of nonzero_bits_mode, which it will be if we've gotten to this
point.

If the current mode is MODE_PARTIAL_INT, then on entry to
reg_nonzero_bits_for_combine, the current nonzero_bits has already been
initialized to GET_MODE_MASK(mode), so we are not at risk of disturbing the
undefined bits, as we are only ever doing &= with GET_MODE_MASK(mode).

However, the above suggestion doesn't actually provide any visible benefit to
testresults, so it doesn't need to be included.

> I don't see how that follows; not all bits in MODE_PARTIAL_INT modes
> are necessarily valid.

Yes, this was an oversight on my part. 
nonzero_bits_mode is only used to calculate last_set_nonzero_bits if
last_set_mode is in the MODE_INT class.
If last_set_mode was MODE_PARTIAL_INT class, then last_set_mode was just that
partial int mode; it wasn't calculated in the wider nonzero_bits_mode.

After some further investigation, it seems we can attribute the recursion to
an inconsistency with how nonzero_bits() is invoked.
The mode passed to nonzero_bits(rtx, mode) is normally the mode of rtx
itself. But there are two cases where nonzero_bits_mode is used instead, even
if that is wider than the mode of the rtx.

In record_value_for_reg:
>   rsp->last_set_mode = mode;
>   if (GET_MODE_CLASS (mode) == MODE_INT
>       && HWI_COMPUTABLE_MODE_P (mode))
>     mode = nonzero_bits_mode;
>   rsp->last_set_nonzero_bits = nonzero_bits (value, mode);

In update_rsp_from_reg_equal: 
>  if (rsp->nonzero_bits != HOST_WIDE_INT_M1U)
>    {
>      bits = nonzero_bits (src, nonzero_bits_mode);

Note that the the mode of src in update_rsp_from_reg_equal is not
checked to see if it is a MODE_INT class and HWI_COMPUTABLE, nonzero_bits_mode
is always used.

This mode passed to nonzero_bits() eventually makes its way to
reg_nonzero_bits_for_combine. rsp->last_set_mode is always the true mode of the
reg (i.e. not nonzero_bits_mode) from when it is set in record_value_for_reg.
So the recursion happens because update_rsp_from_reg_equal has asked for the
nonzero_bits in nonzero_bits_mode, but the last_set_mode was PSImode.
nonzero_bits_mode is not equal to PSImode, nor is it in the same class, so the
nonzero bits will never be reused.

So my revised patch (attached) instead modifies update_rsp_from_reg_equal to
only request nonzero_bits in nonzero_bits_mode if the mode class is MODE_INT
and HWI_COMPUTABLE. This gives it parity with how last_set_nonzero_bits are set
in record_value_for_reg.

I've regtested the attached patch for msp430-elf, currently bootstrapping and
testing on x86_64-pc-linux-gnu.
Is this ok for trunk if bootstrap and regtest for x86_64-pc-linux-gnu is
successful?

Jozef