Re: [RFA:] Fix PR target/18701, address adjustment error withparadoxical mem subregs. (Disallow them, I say!)

[Roger Sayle <roger at eyesopen dot com>]

On Wed, 29 Dec 2004, Hans-Peter Nilsson wrote:
> > Let me give your first patch the attention it deserves.
>
> I'll try to remember to adjust for turkey at this time of the
> year. ;-)

Hi H-P,

Ah, I've now analysed through the "tryptophan haze".  The bit that I
found confusing was your e-mail contained two examples of unsafe
recombinations followed by two patches.  The second example corresponded
to your second patch, with which I agreed and originally approved.
The hangup was that I didn't/don't think the first transformation you
gave is incorrect.  Fortunately, that doesn't matter as your first
patch has nothing to do with your first example!


>	PR target/18701
>	* combine.c (gen_lowpart_for_combine): Re-read the mode of
>	possibly-subreg-stripped x before doing offset calculations for
>	address adjustment.

This is also OK for mainline, but with one minor tweak.  Could you
move your change to correctly update imode and isize from the MEM_P
clause you propose, into the "GET_CODE (x) === SUBREG" clause before
the call to gen_lowpart_common?  Conceptually, its when we change
the value of "x" that we need to update it's mode (imode) and it's
size (isize).

Something like:

<   if (GET_CODE (x) == SUBREG && MEM_P (SUBREG_REG (x)))
<     {
<       x = SUBREG_REG (x);
<       if (GET_MODE (x) == omode)
< 	  return x;
<     }
--
>   if (GET_CODE (x) == SUBREG && MEM_P (SUBREG_REG (x)))
>     {
>       x = SUBREG_REG (x);
>	imode = GET_MODE (x);
>       if (imode == omode)
>         return x;
>       isize = GET_MODE_SIZE (imode);
>     }

but with a suitable comment and ChangeLog entry, blah, blah, blah...

If the above revisions successfully bootstrap and regression test for
you and continue to resolve PR target/18701, consider it preapproved.


We can now resume our scheduled snowball fight on why the middle-end
can't eliminate paradoxical subregs of MEMs until target maintainers
upgrade from LOAD_EXTEND_OP to using explicit zero_extend and/or
sign_extend in their machine descriptions  :>

Thanks,

Roger
--