[RFA,middle-end/54041] Convert modes as needed from expand_expr

expand_expr has, for as long as I can remember, had the ability to 
ignore the desired mode provided by its callers and instead returning 
something in a completely different mode.  It's always been the caller's 
responsibility to deal with that.

For the testcase in 54041, we call expand_expr with a desired mode of 
SImode, but it actually returns a HImode object.  This causes the 
assertion in convert_memory_address_addr_space to trip because the 
passed mode must be the same as the mode of the memory address.

The fix is simple.  If expand_expr returns something in the wrong mode, 
convert it to the desired mode.

I've reviewed the resulting code for the m68k target and it looks 
correct to me.  I've also bootstrapped and regression tested on 
x86_64-unknown-linux-gnu, though the new code most certainly does not 
trigger there.

I guess if someone really wanted to be thorough, they'd test on a target 
where pointers and integers are different sizes.

OK for the trunk?

Thanks,
Jeff

On Thu, Feb 6, 2014 at 8:33 PM, Jeff Law <law@redhat.com> wrote:
>
> expand_expr has, for as long as I can remember, had the ability to ignore
> the desired mode provided by its callers and instead returning something in
> a completely different mode.  It's always been the caller's responsibility
> to deal with that.
>
> For the testcase in 54041, we call expand_expr with a desired mode of
> SImode, but it actually returns a HImode object.  This causes the assertion
> in convert_memory_address_addr_space to trip because the passed mode must be
> the same as the mode of the memory address.
>
> The fix is simple.  If expand_expr returns something in the wrong mode,
> convert it to the desired mode.
>
> I've reviewed the resulting code for the m68k target and it looks correct to
> me.  I've also bootstrapped and regression tested on
> x86_64-unknown-linux-gnu, though the new code most certainly does not
> trigger there.
>
> I guess if someone really wanted to be thorough, they'd test on a target
> where pointers and integers are different sizes.
>
> OK for the trunk?
>
> Thanks,
> Jeff
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 2dbab72..4c7da83 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-02-05  Jeff Law  <law@redhat.com>
> +
> +       PR middle-end/54041
> +       * expr.c (expand_expr_addr_1): Handle expand_expr returning an
> +       object with an undesirable mode.
> +
>  2014-02-05  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>         * config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 878a51b..9609c45 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
> machine_mode tmode,
>                          modifier == EXPAND_INITIALIZER
>                           ? EXPAND_INITIALIZER : EXPAND_NORMAL);
>
> +      /* expand_expr is allowed to return an object in a mode other
> +        than TMODE.  If it did, we need to convert.  */
> +      if (tmode != GET_MODE (tmp))
> +       tmp = convert_modes (tmode, GET_MODE (tmp),
> +                            tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));

What about CONSTANT_P tmp?  Don't you need to use
TYPE_MODE (TREE_TYPE (offset)) in that case?

Richard.

>        result = convert_memory_address_addr_space (tmode, result, as);
>        tmp = convert_memory_address_addr_space (tmode, tmp, as);
>
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index c81a00d..283912d 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-02-05  Jeff Law  <law@redhat.com>
> +
> +       PR middle-end/54041
> +       * gcc.target/m68k/pr54041.c: New test.
> +
>  2014-02-05  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
>
>         * gcc.dg/vmx/sum2s.c: New.
> diff --git a/gcc/testsuite/gcc.target/m68k/pr54041.c
> b/gcc/testsuite/gcc.target/m68k/pr54041.c
> new file mode 100644
> index 0000000..645cb6d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/m68k/pr54041.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -mshort" } */
> +
> +extern int r[];
> +
> +int *fn(int i)
> +{
> +       return &r[i];
> +}
> +
>

On 02/07/14 02:17, Richard Biener wrote:
>> diff --git a/gcc/expr.c b/gcc/expr.c
>> index 878a51b..9609c45 100644
>> --- a/gcc/expr.c
>> +++ b/gcc/expr.c
>> @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
>> machine_mode tmode,
>>                           modifier == EXPAND_INITIALIZER
>>                            ? EXPAND_INITIALIZER : EXPAND_NORMAL);
>>
>> +      /* expand_expr is allowed to return an object in a mode other
>> +        than TMODE.  If it did, we need to convert.  */
>> +      if (tmode != GET_MODE (tmp))
>> +       tmp = convert_modes (tmode, GET_MODE (tmp),
>> +                            tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
>
> What about CONSTANT_P tmp?  Don't you need to use
> TYPE_MODE (TREE_TYPE (offset)) in that case?
Good question.  Given the behaviour of c_m_a_a_s, we should do nothing 
if GET_MODE (tmp) == VOIDmode.  I'll update the patch and resubmit.

jeff